Skip to content

fix: restrict NEA context to Singapore - #78

Merged
IceCodeNew merged 1 commit into
masterfrom
codex/guard-nea-region
Jul 21, 2026
Merged

fix: restrict NEA context to Singapore#78
IceCodeNew merged 1 commit into
masterfrom
codex/guard-nea-region

Conversation

@IceCodeNew

Copy link
Copy Markdown
Owner

Follow-up to #74.

Addresses the remaining region-scope finding in:

Explicit global WEATHER_PROVIDERS now retains nea-sg only for locations identified with country_code == "SG". Mixed configurations fall back to their remaining provider for other locations; nea-sg-only configurations fail with an actionable configuration error instead of injecting Singapore nowcast into another location. The CLI records why an explicitly requested NEA supplement was skipped.

The same old review summary also listed provider-order rewriting. That item was already fixed and resolved in #74 at commit 32194ec: explicit mixed lists must place local capability providers after primary providers, and current tests/documentation enforce that contract. This PR does not duplicate that completed change.

Verification:

  • 790 tests passed with branch coverage
  • line coverage 99.85% (increased from 99.78%)
  • prek run --all-files

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Warning

Review limit reached

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

Next review available in: 34 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: d0899a07-78a4-450e-b29b-6d0b030dd697

📥 Commits

Reviewing files that changed from the base of the PR and between baf5ca8 and 1ca50fe.

📒 Files selected for processing (5)
  • docs/notes.md
  • tests/test_cli.py
  • tests/test_config.py
  • weather_briefing/cli.py
  • weather_briefing/config.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/guard-nea-region

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

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

@IceCodeNew

Copy link
Copy Markdown
Owner Author

/agentic_review

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.78%. Comparing base (baf5ca8) to head (1ca50fe).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #78   +/-   ##
=======================================
  Coverage   99.78%   99.78%           
=======================================
  Files          45       45           
  Lines        8394     8419   +25     
  Branches      488      491    +3     
=======================================
+ Hits         8376     8401   +25     
  Misses         13       13           
  Partials        5        5           

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

@qodo-code-review

qodo-code-review Bot commented Jul 21, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 37 rules

Grey Divider


Remediation recommended

1. NEA region gating undocumented ✓ Resolved 📘 Rule violation ⌂ Architecture
Description
The change adds new behavior that strips or rejects nea-sg when country_code != "SG", but there
is no corresponding architecture note documenting the decision, rationale, trade-offs, and operating
boundaries in docs/notes.md. This can cause future maintainers to unknowingly regress the
region-scope guard or misinterpret configuration failures/warnings.
Code

weather_briefing/config.py[R234-238]

+        if location.country_code != "SG" and WeatherProviderName.NEA_SINGAPORE in configured:
+            available = tuple(provider for provider in configured if provider != WeatherProviderName.NEA_SINGAPORE)
+            if not available:
+                raise ConfigurationError("nea-sg is only available for locations identified as Singapore")
+            return available
Relevance

⭐⭐⭐ High

Team documents provider boundaries in docs/notes.md (NEA supplement notes in #74; capability
semantics clarified in #76).

PR-#74
PR-#76
PR-#63

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR adds new region-scoping logic for nea-sg in the provider resolution path, including a hard
failure when nea-sg is the only configured provider outside Singapore. docs/notes.md contains
detailed architectural boundaries for supplements (and explicitly documents JMA constraints), but
does not include any entry naming nea-sg or documenting this new scoping decision and its
operational boundaries.

Rule 2141673: Document non-obvious architectural decisions in docs/notes.md
weather_briefing/config.py[234-238]
docs/notes.md[5-10]

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 now explicitly scoped to Singapore locations (and is removed / errors outside SG), but this non-obvious architectural boundary is not documented in `docs/notes.md` with rationale, trade-offs, and operating assumptions.

## Issue Context
This PR introduces a new configuration/behavior boundary: `nea-sg` is only valid for `country_code == "SG"`; mixed provider lists skip NEA outside SG and `nea-sg`-only configurations fail with an actionable `ConfigurationError`.

## Fix Focus Areas
- docs/notes.md[5-10]
- weather_briefing/config.py[234-238]
- weather_briefing/cli.py[488-494]

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


Grey Divider

Qodo Logo

Comment thread weather_briefing/config.py
@IceCodeNew
IceCodeNew force-pushed the codex/guard-nea-region branch from e2db956 to 1ca50fe Compare July 21, 2026 03:04
@IceCodeNew

Copy link
Copy Markdown
Owner Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 1ca50fe

@IceCodeNew
IceCodeNew marked this pull request as ready for review July 21, 2026 03:10
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix: gate nea-sg provider to Singapore locations only

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Restrict nea-sg to locations with country_code == "SG" to prevent cross-region nowcast
 leakage.
• Remove NEA from mixed explicit provider lists outside Singapore; error if NEA is sole provider.
• Emit a CLI warning when explicit NEA is skipped; add tests and documentation notes.
Diagram

graph TD
  S["Settings.weather_providers"] --> R["config.weather_providers_for"] --> N["Resolved providers"] --> C["CLI: _weather_context_provider"] --> P["CapabilityProviderSet"]
  L["ResolvedLocation.country_code"] --> R
  R --> E["ConfigurationError"]
  S --> C
  C --> W["Warning: NEA skipped"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Gate NEA inside the `nea-sg` provider implementation
  • ➕ Centralizes eligibility rules with the provider, reducing config-layer branching
  • ➕ Harder to accidentally bypass if provider is instantiated elsewhere
  • ➖ Still needs config/CLI behavior for mixed lists and actionable errors
  • ➖ Less transparent: users may see silent provider failures vs explicit config validation
2. Enable NEA based on verified geographic boundary (polygon/bbox) instead of country_code
  • ➕ Could support Singapore locations even when country_code is missing
  • ➕ More precise if NEA coverage is geographic rather than administrative
  • ➖ Requires maintaining vetted boundary data or a reliable upstream coverage API
  • ➖ Higher risk of false positives/negatives and long-term maintenance burden

Recommendation: Current approach (country_code == "SG" gating in weather_providers_for, plus explicit CLI warning) is the safest and most explainable given NEA’s unbounded Singapore-wide response. It prevents accidental cross-region nowcast injection and provides actionable feedback for both mixed and NEA-only configurations. The boundary/polygon approach is only worth revisiting if a trustworthy coverage signal becomes available.

Files changed (5) +48 / -1

Bug fix (2) +12 / -0
cli.pyLog reason when explicit NEA provider is removed for a location +7/-0

Log reason when explicit NEA provider is removed for a location

• Detects when the user explicitly configured 'nea-sg' but the per-location provider resolution excluded it. Emits a warning with a structured reason (missing country code vs known non-Singapore country) to make skipped supplements observable in CLI runs.

weather_briefing/cli.py

config.pyFilter 'nea-sg' by country_code and fail fast for NEA-only configs +5/-0

Filter 'nea-sg' by country_code and fail fast for NEA-only configs

• Extends 'weather_providers_for()' to remove 'nea-sg' from explicit provider lists when 'country_code != "SG"'. If NEA was the only configured provider, raises 'ConfigurationError' instead of returning a provider set that could inject Singapore nowcast into another location.

weather_briefing/config.py

Tests (2) +34 / -1
test_cli.pyTest CLI warning when explicit NEA supplement is skipped +22/-0

Test CLI warning when explicit NEA supplement is skipped

• Adds a parametrized test ensuring that an explicit (open-meteo, nea-sg) configuration falls back to open-meteo and emits a warning when the location is non-Singapore or lacks a country code. Verifies supplements/metadata are empty when NEA is filtered out.

tests/test_cli.py

test_config.pyTest provider filtering/erroring for NEA outside Singapore +12/-1

Test provider filtering/erroring for NEA outside Singapore

• Updates the multi-supplement ordering test to use an explicit Singapore location ('country_code="SG"'). Adds coverage that mixed lists drop 'nea-sg' outside Singapore and that NEA-only configurations raise a 'ConfigurationError' with an actionable message.

tests/test_config.py

Documentation (1) +2 / -0
notes.mdDocument NEA Singapore-only constraint and rationale +2/-0

Document NEA Singapore-only constraint and rationale

• Adds notes explaining that NEA nowcast is Singapore-wide and must be limited to locations identified as Singapore via 'country_code == "SG"'. Documents the intentional trade-off that missing country metadata disables NEA for safety and outlines what would be needed to relax that constraint in the future.

docs/notes.md

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 1ca50fe

@IceCodeNew
IceCodeNew merged commit 877638e into master Jul 21, 2026
17 of 18 checks passed
@IceCodeNew
IceCodeNew deleted the codex/guard-nea-region branch July 21, 2026 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant