[10/10] refactor: thin the CLI composition root

[IceCodeNew]

Summary

• move provider construction into a dedicated composition package
• reduce cli.py to command dispatch, lifecycle, and scheduling
• remove superseded root compatibility modules
• document the resulting package boundaries and persistence naming

Commits

• refactor: thin the CLI composition root
• docs: document refactored package boundaries
• fix: migrate final reference data consumers

Dependency

Based on #101. Keep draft until #101 is merged into master, then rebase and validate before review.

Verification

• prek run --all-files
• 893 tests passed
• line coverage: 99.87%
• branch coverage: 99.55%
• final tree exactly matches overview commit a45bbc1

Summary by CodeRabbit

• New Features
  • Weather service selection now supports configured fallback providers and optional air-quality information.
  • Delivery options validate required settings for supported publishing channels.
  • External weather, language-model, and delivery services are assembled consistently from application settings.
• Documentation
  • Added documentation describing package responsibilities, supported interfaces, and testing boundaries.
• Refactor
  • Simplified command-line service orchestration and consolidated provider configuration.
  • Updated internal references to use the supported module interfaces.

[coderabbitai]

📝 Walkthrough

Walkthrough

Provider construction moves from cli into composition.providers for LLM, delivery, and weather adapters. Imports and monkeypatch targets now use owning modules, reference data imports use data.resources, and compatibility re-export modules are removed.

Changes

Provider composition refactor

Layer / File(s)	Summary
Adapter composition entry points weather_briefing/composition/*, weather_briefing/composition/providers.py	Adds centralized LLM and delivery provider construction, publisher selection, and Telegram configuration validation.
Weather provider composition weather_briefing/composition/providers.py	Adds weather provider dispatch, capability metadata, fallback composition, regional checks, QWeather validation, and optional AQICN supplementation.
CLI wiring and test targets weather_briefing/cli.py, tests/test_cli.py	Removes provider construction from the CLI and updates tests to import and patch symbols at their owning modules.
Canonical module boundaries weather_briefing/config/environment.py, weather_briefing/geocoding/matching.py, tests/test_geocoding.py, docs/design.md, weather_briefing/{publishers,reference_data,regional_weather,render,weather_context}.py	Moves reference-data imports to data.resources, documents package responsibilities, and removes compatibility re-export layers.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant composition.providers
  participant llm.any_llm
  participant WeatherProviders
  participant DeliveryProvider
  CLI->>composition.providers: request configured adapters
  composition.providers->>llm.any_llm: create structured LLM provider
  composition.providers->>WeatherProviders: build weather and supplement providers
  composition.providers->>DeliveryProvider: build selected publisher
  composition.providers-->>CLI: return composed providers

Loading

Possibly related PRs

• IceCodeNew/weather-briefing#41: Related to the any-LLM structured provider wiring moved into composition.
• IceCodeNew/weather-briefing#71: Related to capability-based weather provider composition and supplementation.
• IceCodeNew/weather-briefing#95: Related to delivery compatibility re-export removal.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 53.85% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: moving composition out of the CLI and thinning the CLI root.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/weather-refactor-10-cli

---

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

[IceCodeNew]

/agentic_review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.82%. Comparing base (e1d9605) to head (127cb12).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #102   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files          91       87    -4     
  Lines        9535     9535           
  Branches      563      563           
=======================================
  Hits         9518     9518           
  Misses         12       12           
  Partials        5        5           

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

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 46 rules

✅ REVIEW.md

1. Diagnostics type boundary leak ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

composition.providers.llm_provider() is typed to accept delivery.RenderedTextDiagnostics but
forwards it to llm.any_llm.create_any_llm_provider(), whose API expects
llm.base.SensitiveLLMDiagnostics. This exposes the wrong abstraction at the LLM composition
boundary and creates an unnecessary dependency from LLM composition onto the delivery package.

Code

weather_briefing/composition/providers.py[R43-55]

+def llm_provider(
+    settings: Settings,
+    diagnostics: RenderedTextDiagnostics | None = None,
+) -> AnyLLMStructuredProvider:
+    """Build the configured any-llm adapter."""
+    return any_llm.create_any_llm_provider(
+        settings.llm_provider,
+        settings.llm_model,
+        settings.llm_max_output_tokens,
+        api_key=settings.api_key,
+        api_base=settings.llm_base_url,
+        diagnostics=diagnostics,
+    )

Relevance

⭐⭐⭐ High

Team often fixes type/contract mismatches; boundary cleanup around SensitiveLLMDiagnostics likely
welcomed.

PR-#24
PR-#96

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new composition-layer llm_provider takes a delivery-scoped diagnostics protocol and passes it
to the LLM adapter factory, whose signature is explicitly LLM-scoped (SensitiveLLMDiagnostics).
This is a boundary/type leak and creates avoidable inter-package coupling.

weather_briefing/composition/providers.py[14-55]
weather_briefing/llm/any_llm.py[150-170]
weather_briefing/llm/base.py[25-31]
weather_briefing/delivery/base.py[31-36]

Agent prompt

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

### Issue description
`weather_briefing/composition/providers.py:llm_provider()` currently accepts `RenderedTextDiagnostics` (from the delivery package) and passes it into `any_llm.create_any_llm_provider()`, which expects `SensitiveLLMDiagnostics` from the LLM package. Even if both protocols happen to share the same method today, this leaks a delivery-layer type into the LLM composition boundary and makes future refactors riskier.

### Issue Context
- `create_any_llm_provider(..., diagnostics=...)` is part of the LLM adapter API and should take an LLM-scoped diagnostics protocol.
- `RenderedTextDiagnostics` lives under `weather_briefing.delivery` and is conceptually delivery-scoped.

### Fix Focus Areas
- weather_briefing/composition/providers.py[14-55]
- weather_briefing/llm/any_llm.py[150-170]
- weather_briefing/llm/base.py[25-31]
- weather_briefing/delivery/base.py[31-36]

### Suggested change
- Change `llm_provider(..., diagnostics: RenderedTextDiagnostics | None = None)` to instead accept `SensitiveLLMDiagnostics | None` (import from `weather_briefing.llm.base` or re-exported from `weather_briefing.llm`).
- Update imports in `composition/providers.py` to remove the delivery dependency for this type.
- If callers currently only have a `RenderedTextDiagnostics` instance, either:
 - pass the same object but type it as `SensitiveLLMDiagnostics` (if that shared protocol is intentional), or
 - introduce an explicit shared protocol in a neutral module (e.g., persistence/diagnostics contracts) that both delivery and llm can depend on.

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

---

2. design.md includes refactor notes ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The new docs/design.md section documents internal package refactor rationale and references to
removed compatibility paths, which is not limited to the current technical contract and embeds
superseded-design narrative. This conflicts with the requirement that docs/design.md remain
contract-focused and avoid historical/previous-approach content.

Code

docs/design.md[R27-40]

+实现按变化原因分包：
+
+- `config` 在环境变量和私密文件边界完成解析与校验；
+- `geocoding` 包含定位协议、候选匹配、外部服务适配器和缓存解析器；
+- `weather` 包含平台无关协议、各天气服务适配器、能力组合和来源文档转换；
+- `llm` 只包含模型协议、结构化 schema、any-llm 兼容适配器和结果解析，不依赖天气领域；
+- `delivery` 分离平台无关投递协议、渲染器和具体平台适配器；
+- `application` 保存历史上下文预算、模型输入构造和输出契约修复等应用策略；
+- `composition` 负责根据配置组装外部服务，`cli` 只负责命令分派、运行生命周期和调度；
+- `persistence` 把 schema、固定格式序列化和运行时诊断与事务存储分开，业务结果仍由单个 `SQLiteStateStore` 原子提交；
+- `data` 保存随程序发布的提示词、端点、分类和本地化资源，读取与领域校验由使用这些资源的功能模块负责。
+
+包的 `__init__` 只导出有意支持的功能接口。实现细节由测试从其所有者模块访问，不通过根级兼容模块维持旧内部路径。
+

Relevance

⭐⭐⭐ High

Repo previously accepted trimming design.md to contract/implementation focus; refactor
rationale/legacy-path narrative likely removed.

PR-#92

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2141667 requires docs/design.md to be limited to the current technical contract
and to avoid embedded narrative about superseded designs. The added section explicitly describes a
refactor-driven packaging rationale and references legacy/compatibility paths, which is outside a
pure contract and resembles 'previous approach' context.

Rule 2141667: Keep docs/design.md limited to the current technical contract
docs/design.md[27-40]

Agent prompt

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

## Issue description
`docs/design.md` gained content that reads like internal refactor/architecture notes (including references to removed compatibility paths), rather than a strict statement of the current technical contract.

## Issue Context
Compliance requires `docs/design.md` to stay focused on the current contract and avoid history/previous-approach narrative. The added section explains packaging “by change reason” and mentions not keeping old internal paths via compatibility modules.

## Fix Focus Areas
- docs/design.md[27-40]

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

---

3. create_any_llm_provider patched at import-site ✓ Resolved 📘 Rule violation ▣ Testability

Description

The updated tests patch create_any_llm_provider and weather_providers_for via
weather_briefing.composition.providers, even though those functions are defined in
weather_briefing.llm.any_llm and weather_briefing.config.environment respectively. This violates
the compliance rule requiring patches to target the symbol’s defining module rather than an import
site.

Code

tests/test_cli.py[R1105-1108]

        monkeypatch.setattr(
-            "weather_briefing.cli.create_any_llm_provider",
+            "weather_briefing.composition.providers.create_any_llm_provider",
            lambda *args, **kwargs: calls.append((args, kwargs)) or sdk_client,
        )

Relevance

⭐⭐⭐ High

Multiple accepted precedents require patching at defining module, not import-site bindings, to
reduce test brittleness.

PR-#101
PR-#99
PR-#95

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2274647 requires that patch targets point to the module where the symbol is
defined. The cited tests use monkeypatch.setattr() against
weather_briefing.composition.providers.create_any_llm_provider and
weather_briefing.composition.providers.weather_providers_for, but the actual function definitions
are located in weather_briefing/llm/any_llm.py (def create_any_llm_provider(...)) and
weather_briefing/config/environment.py (def weather_providers_for(...)), demonstrating the patch
paths are import sites rather than defining modules.

Rule 2274647: Patch behavior at its defining module, not where it is imported
tests/test_cli.py[1105-1108]
tests/test_cli.py[1129-1132]
tests/test_cli.py[1141-1144]
weather_briefing/llm/any_llm.py[150-171]
tests/test_cli.py[1329-1332]
weather_briefing/config/environment.py[164-178]

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 tests are using `monkeypatch.setattr()` to patch `create_any_llm_provider` and `weather_providers_for` via an import-site module path (`weather_briefing.composition.providers.*`) instead of patching the functions at their defining module paths, which violates compliance requirements.

## Issue Context
Compliance (PR Compliance ID 2274647) requires patching the symbol in the module where it is defined. In this codebase, `create_any_llm_provider` is defined in `weather_briefing.llm.any_llm` (file `weather_briefing/llm/any_llm.py`), and `weather_providers_for` is defined in `weather_briefing.config.environment` (file `weather_briefing/config/environment.py`). Update the test patch targets to reference those defining modules rather than `weather_briefing.composition.providers`.

## Fix Focus Areas
- tests/test_cli.py[1105-1108]
- tests/test_cli.py[1129-1132]
- tests/test_cli.py[1141-1144]
- tests/test_cli.py[1329-1332]
- weather_briefing/llm/any_llm.py[150-171]
- weather_briefing/config/environment.py[164-178]

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

---

Previous review results

Review updated until commit 127cb12

Results up to commit 6f2da79 ⚖️ Balanced

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

1. design.md includes refactor notes ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The new docs/design.md section documents internal package refactor rationale and references to
removed compatibility paths, which is not limited to the current technical contract and embeds
superseded-design narrative. This conflicts with the requirement that docs/design.md remain
contract-focused and avoid historical/previous-approach content.

Code

docs/design.md[R27-40]

+实现按变化原因分包：
+
+- `config` 在环境变量和私密文件边界完成解析与校验；
+- `geocoding` 包含定位协议、候选匹配、外部服务适配器和缓存解析器；
+- `weather` 包含平台无关协议、各天气服务适配器、能力组合和来源文档转换；
+- `llm` 只包含模型协议、结构化 schema、any-llm 兼容适配器和结果解析，不依赖天气领域；
+- `delivery` 分离平台无关投递协议、渲染器和具体平台适配器；
+- `application` 保存历史上下文预算、模型输入构造和输出契约修复等应用策略；
+- `composition` 负责根据配置组装外部服务，`cli` 只负责命令分派、运行生命周期和调度；
+- `persistence` 把 schema、固定格式序列化和运行时诊断与事务存储分开，业务结果仍由单个 `SQLiteStateStore` 原子提交；
+- `data` 保存随程序发布的提示词、端点、分类和本地化资源，读取与领域校验由使用这些资源的功能模块负责。
+
+包的 `__init__` 只导出有意支持的功能接口。实现细节由测试从其所有者模块访问，不通过根级兼容模块维持旧内部路径。
+

Relevance

⭐⭐⭐ High

Repo previously accepted trimming design.md to contract/implementation focus; refactor
rationale/legacy-path narrative likely removed.

PR-#92

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2141667 requires docs/design.md to be limited to the current technical contract
and to avoid embedded narrative about superseded designs. The added section explicitly describes a
refactor-driven packaging rationale and references legacy/compatibility paths, which is outside a
pure contract and resembles 'previous approach' context.

Rule 2141667: Keep docs/design.md limited to the current technical contract
docs/design.md[27-40]

Agent prompt

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

## Issue description
`docs/design.md` gained content that reads like internal refactor/architecture notes (including references to removed compatibility paths), rather than a strict statement of the current technical contract.

## Issue Context
Compliance requires `docs/design.md` to stay focused on the current contract and avoid history/previous-approach narrative. The added section explains packaging “by change reason” and mentions not keeping old internal paths via compatibility modules.

## Fix Focus Areas
- docs/design.md[27-40]

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

---

2. create_any_llm_provider patched at import-site ✓ Resolved 📘 Rule violation ▣ Testability

Description

The updated tests patch create_any_llm_provider and weather_providers_for via
weather_briefing.composition.providers, even though those functions are defined in
weather_briefing.llm.any_llm and weather_briefing.config.environment respectively. This violates
the compliance rule requiring patches to target the symbol’s defining module rather than an import
site.

Code

tests/test_cli.py[R1105-1108]

        monkeypatch.setattr(
-            "weather_briefing.cli.create_any_llm_provider",
+            "weather_briefing.composition.providers.create_any_llm_provider",
            lambda *args, **kwargs: calls.append((args, kwargs)) or sdk_client,
        )

Relevance

⭐⭐⭐ High

Multiple accepted precedents require patching at defining module, not import-site bindings, to
reduce test brittleness.

PR-#101
PR-#99
PR-#95

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2274647 requires that patch targets point to the module where the symbol is
defined. The cited tests use monkeypatch.setattr() against
weather_briefing.composition.providers.create_any_llm_provider and
weather_briefing.composition.providers.weather_providers_for, but the actual function definitions
are located in weather_briefing/llm/any_llm.py (def create_any_llm_provider(...)) and
weather_briefing/config/environment.py (def weather_providers_for(...)), demonstrating the patch
paths are import sites rather than defining modules.

Rule 2274647: Patch behavior at its defining module, not where it is imported
tests/test_cli.py[1105-1108]
tests/test_cli.py[1129-1132]
tests/test_cli.py[1141-1144]
weather_briefing/llm/any_llm.py[150-171]
tests/test_cli.py[1329-1332]
weather_briefing/config/environment.py[164-178]

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 tests are using `monkeypatch.setattr()` to patch `create_any_llm_provider` and `weather_providers_for` via an import-site module path (`weather_briefing.composition.providers.*`) instead of patching the functions at their defining module paths, which violates compliance requirements.

## Issue Context
Compliance (PR Compliance ID 2274647) requires patching the symbol in the module where it is defined. In this codebase, `create_any_llm_provider` is defined in `weather_briefing.llm.any_llm` (file `weather_briefing/llm/any_llm.py`), and `weather_providers_for` is defined in `weather_briefing.config.environment` (file `weather_briefing/config/environment.py`). Update the test patch targets to reference those defining modules rather than `weather_briefing.composition.providers`.

## Fix Focus Areas
- tests/test_cli.py[1105-1108]
- tests/test_cli.py[1129-1132]
- tests/test_cli.py[1141-1144]
- tests/test_cli.py[1329-1332]
- weather_briefing/llm/any_llm.py[150-171]
- weather_briefing/config/environment.py[164-178]

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

---

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 6f2da79

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0d32253

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 127cb12

[qodo-code-review]

Code review by qodo was updated up to the latest commit 127cb12

[qodo-code-review]

PR Summary by Qodo

Refactor: move provider composition out of the CLI entrypoint

✨ Enhancement 📝 Documentation 🧪 Tests 🕐 40+ Minutes

AI Description

• Extract LLM/weather/delivery provider construction into a dedicated composition module.
• Reduce cli.py to command dispatch, lifecycle management, and scheduling concerns.
• Remove compatibility re-export modules and update imports, tests, and design docs.

Diagram

graph TD
  cli["weather_briefing/cli.py"] --> comp["composition/providers.py"] --> weather["weather providers"] --> ext{{"External APIs"}}
  comp --> delivery["delivery publishers"] --> ext
  comp --> llm["llm/any_llm"] --> ext
  comp --> config[("config/environment.py")]

  subgraph Legend
    direction LR
    _mod["Module/File"] ~~~ _cfg[("Config")] ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Push provider factories down into domain packages

• ➕ Keeps construction logic owned by weather/, delivery/, llm/ maintainers
• ➕ Reduces size/centrality of composition/providers.py over time
• ➖ Composition wiring becomes more scattered; harder to see full runtime graph
• ➖ CLI may need more cross-package imports unless an intermediate facade exists

2. Introduce a lightweight DI container/wiring framework

• ➕ Clear override points for tests and environments
• ➕ Centralized lifecycle management (scopes, singletons)
• ➖ Adds dependency/indirection overhead for a small codebase
• ➖ Can obscure straightforward construction paths and complicate debugging

Recommendation: The PR’s approach is a good fit: it creates an explicit composition boundary without adding framework overhead, and keeps cli.py focused on orchestration. If composition/providers.py grows further, consider splitting it by concern (llm/delivery/weather) while keeping composition as the single public wiring surface.

Files changed (8) +370 / -358

Refactor (5) +338 / -342

cli.pyThin CLI by delegating provider wiring to composition module +6/-340

Thin CLI by delegating provider wiring to composition module

• Removes in-file provider factory/builder logic (LLM, delivery publishers, weather provider composition) and imports equivalent functions from 'composition.providers'. Keeps CLI focused on argument parsing, runtime lifecycle, scheduling, and per-location execution paths.

weather_briefing/cli.py

__init__.pyAdd composition package marker +1/-0

Add composition package marker

• Introduces the 'composition' package to host dependency wiring and composition-root utilities.

weather_briefing/composition/init.py

providers.pyCentralize runtime construction for LLM, delivery, and weather providers +329/-0

Centralize runtime construction for LLM, delivery, and weather providers

• Adds a new composition module that builds the configured any-llm provider, delivery provider (stdout/telegram), and weather context providers (including fallback/supplements and metadata). Encapsulates provider selection rules, capability metadata, and configuration validation (e.g., QWeather JWT presence).

weather_briefing/composition/providers.py

environment.pySwitch reference data tuple loader import to data resources +1/-1

Switch reference data tuple loader import to data resources

• Migrates 'reference_string_tuple' import to 'weather_briefing.data.resources', aligning with removal of the old compatibility export module.

weather_briefing/config/environment.py

matching.pyUpdate reference data imports for geocoding matching +1/-1

Update reference data imports for geocoding matching

• Moves geocoding matching reference lookups ('ReferenceDataError', 'reference_string_tuple', 'reference_value') to 'weather_briefing.data.resources' to match the new package boundary.

weather_briefing/geocoding/matching.py

Tests (2) +18 / -16

test_cli.pyUpdate CLI tests to import provider builders from composition +17/-15

Update CLI tests to import provider builders from composition

• Moves test imports for provider factories/builders from 'weather_briefing.cli' to 'weather_briefing.composition.providers'. Updates monkeypatch targets for any-llm construction and environment provider selection, and switches 'QWeatherProvider' import to the 'weather' package.

tests/test_cli.py

test_geocoding.pyMigrate reference data imports to data resources module +1/-1

Migrate reference data imports to data resources module

• Updates tests to import 'ReferenceDataError' and 'reference_value' from 'weather_briefing.data.resources' instead of the removed compatibility module.

tests/test_geocoding.py

Documentation (1) +14 / -0

design.mdDocument package responsibilities and new composition boundary +14/-0

Document package responsibilities and new composition boundary

• Adds an explicit package responsibility list, including 'composition' owning provider assembly and 'cli' owning dispatch/lifecycle/scheduling. Clarifies 'persistence' responsibilities and '__init__' export expectations.

docs/design.md

[IceCodeNew]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (1)

weather_briefing/composition/providers.py (1)

252-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Redundant builder registry vs. special-casing in build_weather_provider.

WEATHER_PROVIDER_BUILDERS is fetched into builder, but that value is only invoked for the fall-through (Open-Meteo) case — QWeather/NEA/JMA are special-cased above, so the dict acts mostly as an existence check. The NEA branch (Line 267) also duplicates _build_nea verbatim. Consider either dispatching through the registry uniformly (e.g., normalizing all builders to a common signature that accepts output_language/jma_office_code) or dropping the unused registry to remove the duplication.

🤖 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/composition/providers.py` around lines 252 - 329, Simplify
provider construction in build_weather_provider by removing the redundant
WEATHER_PROVIDER_BUILDERS lookup and registry, or replace the special-case
branches with uniform registry dispatch. Preserve QWeather output_language
handling, JMA jma_office_code validation, and the existing NEA/Open-Meteo
construction behavior while eliminating the duplicated _build_nea path.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@weather_briefing/composition/providers.py`:
- Around line 252-329: Simplify provider construction in build_weather_provider
by removing the redundant WEATHER_PROVIDER_BUILDERS lookup and registry, or
replace the special-case branches with uniform registry dispatch. Preserve
QWeather output_language handling, JMA jma_office_code validation, and the
existing NEA/Open-Meteo construction behavior while eliminating the duplicated
_build_nea path.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 292f8cef-fc98-4dcf-b114-f2110a66806c

📥 Commits

Reviewing files that changed from the base of the PR and between e1d9605 and 127cb12.

📒 Files selected for processing (13)

• docs/design.md
• tests/test_cli.py
• tests/test_geocoding.py
• weather_briefing/cli.py
• weather_briefing/composition/__init__.py
• weather_briefing/composition/providers.py
• weather_briefing/config/environment.py
• weather_briefing/geocoding/matching.py
• weather_briefing/publishers.py
• weather_briefing/reference_data.py
• weather_briefing/regional_weather.py
• weather_briefing/render.py
• weather_briefing/weather_context.py

💤 Files with no reviewable changes (5)

• weather_briefing/publishers.py
• weather_briefing/regional_weather.py
• weather_briefing/weather_context.py
• weather_briefing/reference_data.py
• weather_briefing/render.py

[IceCodeNew]

The CodeRabbit Nitpick in review 4763255472 is valid. #106 removes the redundant weather builder registry and duplicated NEA construction in a focused follow-up because #102 was already merged.