refactor: introduce weather capability composition

[IceCodeNew]

What changed

Introduce a composable capability layer for weather, air quality, and supplementary regional providers. Existing weather providers now run through the new abstraction while Open-Meteo remains the primary weather source.

Why

This establishes the extension boundary needed to add regional data sources without growing provider-specific conditionals in the briefing service.

Validation

• Full test suite passed on the complete stack: 722 tests
• Every stacked commit passed full branch coverage and repository hooks independently
• Ruff, formatting, ty, secret, and workflow checks passed

Summary by CodeRabbit

• New Features

  • Added capability-based weather context support, including weather, air quality, allergen, lifestyle, alert, and nowcast capabilities.
  • Improved provider selection and fallback behavior based on supported capabilities.
  • Added optional air-quality supplementation for current weather data when available.
  • Added clearer errors for unavailable air-quality data, failed fallbacks, and unsupported forecast-date requests.

• Documentation

  • Updated design and domain documentation to describe capability composition, provider responsibilities, and future extension guidelines.

• Tests

  • Added and expanded coverage for capability metadata, fallback behavior, air-quality handling, and dated forecasts.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds capability-based provider composition for weather contexts, including metadata, optional AQICN air-quality supplementation, dated-context routing, CLI wiring, documentation, and async test coverage.

Changes

Capability composition

Layer / File(s)	Summary
Capability contracts and context composition weather_briefing/capabilities.py, docs/design.md, docs/notes.md	Defines capability identifiers and metadata, composes weather and air-quality providers, routes dated contexts, and documents provider-slot behavior.
CLI capability metadata wiring weather_briefing/cli.py	Maps active weather providers to shared capabilities and constructs CapabilityProviderSet with optional AQICN support.
Capability behavior validation tests/test_capabilities.py, tests/test_cli.py	Covers metadata reporting, current and dated contexts, fallback errors, provider selection, and unknown-provider validation.

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

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant CapabilityProviderSet
  participant WeatherProvider
  participant AQICNProvider
  CLI->>CapabilityProviderSet: construct provider composition
  CapabilityProviderSet->>WeatherProvider: fetch weather context
  WeatherProvider-->>CapabilityProviderSet: WeatherContextSnapshot
  CapabilityProviderSet->>AQICNProvider: fetch missing current air quality
  AQICNProvider-->>CapabilityProviderSet: AirQualitySnapshot
  CapabilityProviderSet-->>CLI: composed weather context

Loading

Suggested labels: 🕐 40+ Minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% 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 clearly matches the main change: introducing capability-based weather provider composition.
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/pr1-capabilities

---

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.76%. Comparing base (b73fbf7) to head (ecf4e29).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff            @@
##           master      #71    +/-   ##
========================================
  Coverage   99.75%   99.76%            
========================================
  Files          39       41     +2     
  Lines        7424     7580   +156     
  Branches      416      425     +9     
========================================
+ Hits         7406     7562   +156     
  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): 31 rules

✅ REVIEW.md

1. Metadata registry can drift ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

_weather_provider_metadata() directly indexes _WEATHER_PROVIDER_METADATA for every active provider
name, so adding a new weather provider without also updating this parallel metadata registry will
crash provider construction with KeyError. This creates a fragile extension point precisely where
the PR is introducing provider composition as an extensibility boundary.

Code

weather_briefing/cli.py[R440-452]

+def _weather_provider_metadata(names: Sequence[str]) -> ProviderCapabilities:
+    """Describe capabilities common to every active fallback provider."""
+    metadata = [_WEATHER_PROVIDER_METADATA[name] for name in names]
+    if len(metadata) == 1:
+        return metadata[0]
+    capabilities = metadata[0].capabilities
+    for item in metadata[1:]:
+        capabilities &= item.capabilities
+    return ProviderCapabilities(
+        provider_id="weather-composite",
+        provider_name="Weather provider composite",
+        capabilities=capabilities,
+    )

Relevance

⭐⭐⭐ High

Team previously fixed KeyError crashes via explicit contract validation/errors (PR35, PR52); likely
accept drift-proofing metadata lookup.

PR-#35
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

_weather_provider_metadata() uses direct dict indexing for each provider name, so any missing
entry will raise KeyError. Provider names are validated against the supported provider set, but
the metadata registry is a separate structure that must be kept in sync when adding/renaming
providers, otherwise the CLI crashes during provider assembly.

weather_briefing/cli.py[414-452]
weather_briefing/config.py[28-29]
weather_briefing/config.py[183-193]
weather_briefing/registries.py[6-11]

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_provider_metadata()` assumes every active provider id exists in `_WEATHER_PROVIDER_METADATA` and performs direct indexing. If a new weather provider is added to the supported/constructed provider set but metadata isn’t updated, CLI construction will raise `KeyError` instead of producing a clear configuration/developer error.

## Issue Context
There are now two parallel registries in `cli.py`: one for construction (`WEATHER_PROVIDER_BUILDERS`) and one for capability metadata (`_WEATHER_PROVIDER_METADATA`). They can drift independently.

## Fix Focus Areas
- weather_briefing/cli.py[414-452]

## Suggested fix
1. Make `_weather_provider_metadata()` robust:
  - Use `_WEATHER_PROVIDER_METADATA.get(name)` and raise a `ValueError` (or `ConfigurationError`) with a clear message if missing.
2. Add an internal consistency check at import time (or in a small helper/test):
  - Assert that `set(WEATHER_PROVIDER_BUILDERS)` (or `SUPPORTED_WEATHER_PROVIDERS`) is a subset of `_WEATHER_PROVIDER_METADATA` keys, so drift is caught immediately during development.
3. (Optional) Add a unit test that would fail if a supported provider is missing metadata.

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

---

2. Dated dispatch may TypeError ✓ Resolved 🐞 Bug ☼ Reliability

Description

_fetch_context() uses getattr(provider, "fetch_for_date") and then blindly awaits it, so a provider
with a non-callable or incompatible fetch_for_date attribute will raise a TypeError instead of the
expected WeatherContextError.
This diverges from the existing fetch_weather_context() contract which gates dated support via
DatedWeatherContextProvider and consistently raises WeatherContextError when unsupported.

Code

weather_briefing/capabilities.py[R98-112]

+async def _fetch_context(
+    provider: ContextCapabilityProvider,
+    latitude: float,
+    longitude: float,
+    forecast_date: pendulum.Date | None,
+) -> WeatherContextSnapshot:
+    """Call providers that support either current or dated context."""
+    if forecast_date is None:
+        return await provider.fetch(latitude, longitude)
+    fetch_for_date = getattr(provider, "fetch_for_date", None)
+    if fetch_for_date is None:
+        from .weather_context import WeatherContextError
+
+        raise WeatherContextError(f"{type(provider).__name__} does not support target forecast dates")
+    return await fetch_for_date(latitude, longitude, forecast_date)

Relevance

⭐⭐⭐ High

Team wraps provider contract failures into WeatherContextError rather than leaking
TypeError/KeyError (PR #35, #37).

PR-#35
PR-#37
PR-#50

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

_fetch_context() only checks for presence of fetch_for_date and then calls it, which can raise
TypeError if the attribute exists but is not a compatible async callable. The existing
weather-context boundary uses DatedWeatherContextProvider + isinstance() to ensure unsupported
providers fail with WeatherContextError instead of leaking unrelated exception types.

weather_briefing/capabilities.py[98-112]
weather_briefing/weather_context.py[59-71]
weather_briefing/weather_context.py[665-676]

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.capabilities._fetch_context()` detects dated support via `getattr(provider, "fetch_for_date", None)` and then awaits the result without validating it is callable / conforms to the dated-provider contract. This can leak a raw `TypeError` (or other unexpected exception) instead of raising the domain-specific `WeatherContextError` used elsewhere.

### Issue Context
The codebase already has a dated-provider boundary defined in `weather_briefing.weather_context` via `DatedWeatherContextProvider` and `fetch_weather_context()`, which raises `WeatherContextError` when dated fetch is not supported. Aligning `_fetch_context()` to that contract avoids inconsistent behavior and prevents unexpected exception types.

### Fix Focus Areas
- weather_briefing/capabilities.py[98-112]

### Suggested fix
In `_fetch_context()`, replace the `getattr()` approach with the existing contract check:
- lazily import `DatedWeatherContextProvider` + `WeatherContextError` (to keep import boundaries similar to current code)
- `if not isinstance(provider, DatedWeatherContextProvider): raise WeatherContextError(...)`
- otherwise `return await provider.fetch_for_date(latitude, longitude, forecast_date)`

(Alternative minimal fix if you want to keep duck-typing: check `callable(fetch_for_date)` before calling, and raise `WeatherContextError` if not callable.)

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

---

3. CapabilityProviderSet missing notes entry ✓ Resolved 📘 Rule violation ⌂ Architecture

Description

The PR introduces a non-obvious architectural change (capability composition via
CapabilityProviderSet) but does not add a corresponding rationale/trade-offs/boundaries entry in
docs/notes.md. This leaves the new extension boundary undocumented in the repository’s designated
architecture notes.

Code

weather_briefing/capabilities.py[R52-86]

+@dataclass(frozen=True, slots=True)
+class CapabilityProviderSet:
+    """Compose independent weather and air-quality capabilities."""
+
+    weather: ContextCapabilityProvider
+    weather_metadata: ProviderCapabilities
+    air_quality: AirQualityProvider | None = None
+    air_quality_metadata: ProviderCapabilities | None = None
+
+    async def fetch(
+        self,
+        latitude: float,
+        longitude: float,
+        *,
+        forecast_date: pendulum.Date | None = None,
+    ) -> WeatherContextSnapshot:
+        """Fetch weather and fill a missing current air-quality capability."""
+        snapshot = await _fetch_context(self.weather, latitude, longitude, forecast_date)
+        if snapshot.air_quality is not None or forecast_date is not None:
+            return snapshot
+        if self.air_quality is None:
+            from .weather_context import WeatherContextError
+
+            raise WeatherContextError("Weather source did not provide air quality; configure AQICN_API_TOKEN")
+        try:
+            air_quality = await self.air_quality.fetch(
+                latitude,
+                longitude,
+                datetime_timezone_specifier(snapshot.observed_at, context="Weather snapshot time"),
+            )
+        except AirQualityError:
+            from .weather_context import WeatherContextError
+
+            raise WeatherContextError("Weather source did not provide air quality and AQICN fallback failed") from None
+        return replace(snapshot, air_quality=air_quality)

Relevance

⭐⭐⭐ High

docs/notes.md is actively used for non-obvious architecture rationale; prior PRs added/expanded
notes entries for boundaries/trade-offs.

PR-#37
PR-#54
PR-#58

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

CapabilityProviderSet is introduced as a new composition/extension boundary in code, while
docs/notes.md (the designated place for non-obvious architectural decisions) contains no matching
entry documenting this decision’s rationale, trade-offs, or operating boundaries. This violates the
requirement to document such decisions in docs/notes.md.

Rule 2141673: Document non-obvious architectural decisions in docs/notes.md
weather_briefing/capabilities.py[52-86]
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
A new capability composition boundary (`CapabilityProviderSet`) is introduced, but `docs/notes.md` does not include an entry documenting the decision, rationale, trade-offs, and operating boundaries/assumptions.

## Issue Context
`docs/notes.md` is explicitly intended to capture key architectural decisions whose rationale is not obvious from the external contract. Capability composition affects how providers are extended and how missing capabilities (like air quality) are supplemented.

## Fix Focus Areas
- weather_briefing/capabilities.py[52-96]
- weather_briefing/cli.py[415-463]
- docs/notes.md[1-50]

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

---

View more (2)

4. AirQualitySupplementingWeatherProvider doc is stale ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

docs/design.md still describes AirQualitySupplementingWeatherProvider as the current air-quality
supplementation mechanism, but the CLI now wires weather context through CapabilityProviderSet.
This makes the design doc describe a superseded contract as current, which can mislead maintainers
and implementers.

Code

docs/design.md[R51-52]

+能力组合边界由 `capabilities.py` 的 `CapabilityProviderSet` 承担。天气、空气质量、过敏原、生活指数、预警和短时预报属于可独立声明的 capability；现有 QWeather/Open-Meteo 完整上下文 adapter 暂时挂在天气槽位，AQICN 挂在空气质量槽位。这样本地气象机构可以只实现预警或 nowcast，而不必伪装为完整天气 provider；后续能力 provider 不应为填充无关字段而发起额外请求。
+

Relevance

⭐⭐⭐ High

Team previously accepted doc-drift fixes in docs (avoid duplication/drift) and frequently updates
design.md with refactors.

PR-#62
PR-#63
PR-#59

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The documentation describes AirQualitySupplementingWeatherProvider as the current mechanism, while
the updated CLI composition constructs and returns CapabilityProviderSet instead. This violates
the requirement that modified docs under docs/ must not present superseded designs as current
without clearly marking them as historical/deprecated.

Rule 2141667: Docs in docs/ must not describe superseded contracts as current
docs/design.md[49-60]
weather_briefing/cli.py[415-463]

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` currently documents `AirQualitySupplementingWeatherProvider` as the active air-quality supplementation path, but the implementation wiring in `weather_briefing/cli.py` now uses `CapabilityProviderSet`.

## Issue Context
This PR introduces capability composition and updates the runtime composition root accordingly. The design doc should describe the current contract, and any legacy approach must be explicitly marked as historical/deprecated.

## Fix Focus Areas
- docs/design.md[49-60]
- weather_briefing/cli.py[415-463]

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

---

5. Wrong capability metadata ✓ Resolved 🐞 Bug ≡ Correctness

Description

_weather_context_provider() hard-codes weather_metadata.capabilities to include
LIFESTYLE/AIR_QUALITY/ALLERGEN even though the configured underlying weather adapters do not
consistently populate those normalized fields, so ProviderCapabilities.supports() can return
incorrect results. This violates the contract implied by ProviderCapabilities (“capabilities
exposed by one provider adapter”) and can mislead any capability-based routing/validation that uses
this metadata.

Code

weather_briefing/cli.py[R440-450]

+        weather_metadata=ProviderCapabilities(
+            provider_id="weather-composite",
+            provider_name="Weather provider composite",
+            capabilities=frozenset(
+                {
+                    CapabilityName.WEATHER,
+                    CapabilityName.LIFESTYLE,
+                    CapabilityName.AIR_QUALITY,
+                    CapabilityName.ALLERGEN,
+                }
+            ),

Relevance

⭐⭐ Medium

Team enforces correctness/contract alignment (accepted similar contract-metadata/type fixes), but no
direct history on ProviderCapabilities accuracy.

PR-#24
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The CLI declares broad capabilities on weather_metadata, but provider implementations show those
fields are not consistently provided in the normalized snapshot: Open-Meteo does not set
lifestyle_advice, and QWeather does not set the normalized allergen field. This makes
ProviderCapabilities.supports() an unreliable indicator of actual support.

weather_briefing/cli.py[415-462]
weather_briefing/capabilities.py[39-50]
weather_briefing/weather_context.py[312-321]
weather_briefing/weather_context.py[459-472]
weather_briefing/models.py[152-165]

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_context_provider()` currently constructs `ProviderCapabilities` for the composed weather provider with a hard-coded capability set that does not match what the selected provider(s) actually expose via the normalized `WeatherContextSnapshot` fields.

## Issue Context
- `ProviderCapabilities` is documented as describing capabilities exposed by one provider adapter.
- Open-Meteo snapshots do not populate `lifestyle_advice` at all (it stays the default empty tuple), so advertising `CapabilityName.LIFESTYLE` on the weather adapter is incorrect when Open-Meteo is the chosen weather provider.
- QWeather snapshots do not populate the normalized `allergen` snapshot field (only `allergen_advice_available`), so advertising `CapabilityName.ALLERGEN` as a weather adapter capability is incorrect.

## Fix Focus Areas
- weather_briefing/cli.py[415-462]
- weather_briefing/capabilities.py[39-60]

## Suggested approach
1. Replace the hard-coded `capabilities=frozenset({...})` with capabilities derived from the actual configured weather provider(s) (e.g., mapping from provider name/type to capability set), or
2. If the intent is to describe aggregate capabilities of the entire `CapabilityProviderSet`, introduce a separate metadata record for the composed set (e.g., `composed_metadata`) and keep `weather_metadata` limited to the weather adapter’s real capabilities.
3. Ensure `CapabilityName` entries correspond to the normalized snapshot fields that are actually populated (e.g., only claim `ALLERGEN` when `snapshot.allergen` is supported/populated).

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

---

6. Duplicated dated dispatch 🐞 Bug ⚙ Maintainability ⭐ New

Description

CapabilityProviderSet introduces _fetch_context() that re-implements the same “current vs
forecast_date” dispatch already provided by weather_context.fetch_weather_context(), creating two
sources of truth that can drift in behavior and error semantics. This is especially risky because
both paths are exercised in production (service.py uses fetch_weather_context(), while
CapabilityProviderSet uses _fetch_context()).

Code

weather_briefing/capabilities.py[R98-114]

+async def _fetch_context(
+    provider: ContextCapabilityProvider,
+    latitude: float,
+    longitude: float,
+    forecast_date: pendulum.Date | None,
+) -> WeatherContextSnapshot:
+    """Call providers that support either current or dated context."""
+    if forecast_date is None:
+        return await provider.fetch(latitude, longitude)
+    from .weather_context import DatedWeatherContextProvider, WeatherContextError
+
+    if not isinstance(provider, DatedWeatherContextProvider):
+        raise WeatherContextError(f"{type(provider).__name__} does not support target forecast dates")
+    fetch_for_date = provider.fetch_for_date
+    if not callable(fetch_for_date):
+        raise WeatherContextError(f"{type(provider).__name__} does not support target forecast dates")
+    return await fetch_for_date(latitude, longitude, forecast_date)

Relevance

⭐⭐ Medium

No direct precedent on deduping dispatch; repo recently tightened target-date semantics, so drift
risk likely taken seriously.

PR-#50
PR-#22

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR adds _fetch_context() in capabilities.py, while the existing fetch_weather_context() in
weather_context.py already performs the current-vs-dated dispatch and is still used by
BriefingService. This means dispatch behavior is now defined in two places, increasing drift risk.

weather_briefing/capabilities.py[61-114]
weather_briefing/weather_context.py[665-676]
weather_briefing/service.py[331-338]

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/capabilities.py` defines `_fetch_context()` to route between `fetch()` and `fetch_for_date()`, but the repo already has `weather_briefing/weather_context.py:fetch_weather_context()` that does the same routing. Keeping both increases the chance of future drift (e.g., one adds new validation or different error messages while the other doesn’t).

### Issue Context
- `CapabilityProviderSet.fetch()` currently calls `_fetch_context()`.
- `BriefingService` still calls `fetch_weather_context()` directly.

### Fix Focus Areas
- Prefer a single shared dispatch implementation (either call `fetch_weather_context()` from `capabilities.py`, or move dispatch into a shared helper used by both).
- If the callable-guard behavior is desired, consider adding it to the shared helper so both code paths behave consistently.

- weather_briefing/capabilities.py[61-114]
- weather_briefing/weather_context.py[665-676]
- weather_briefing/service.py[331-338]

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

---

7. QWeather allergen metadata mismatch 🐞 Bug ≡ Correctness

Description

_WEATHER_PROVIDER_METADATA marks QWeather as not supporting CapabilityName.ALLERGEN, but
QWeatherProvider can set WeatherContextSnapshot.allergen_advice_available and
snapshot_to_documents() exposes that as SourceDocument.has_allergen_information. This makes
ProviderCapabilities.supports(ALLERGEN) inconsistent with what the QWeather adapter can actually
emit, so supports() cannot be trusted to reflect allergen info availability for QWeather.

Code

weather_briefing/cli.py[R414-425]

+_WEATHER_PROVIDER_METADATA: dict[str, ProviderCapabilities] = {
+    WeatherProviderName.QWEATHER: ProviderCapabilities(
+        provider_id=WeatherProviderName.QWEATHER,
+        provider_name="QWeather",
+        capabilities=frozenset(
+            {
+                CapabilityName.WEATHER,
+                CapabilityName.AIR_QUALITY,
+                CapabilityName.LIFESTYLE,
+            }
+        ),
+    ),

Relevance

⭐ Low

Team treats QWeather allergen as lifestyle advice; tests/docs enforce conservative capabilities. See
allergen work in PRs 23/31.

PR-#23
PR-#31

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The CLI metadata omits ALLERGEN for QWeather, but the existing QWeather adapter computes
allergen_advice_available from the indices response and snapshot_to_documents() turns that into
has_allergen_information on the produced SourceDocument. This demonstrates QWeather can expose
allergen-related context even though the new supports(ALLERGEN) metadata reports otherwise.

weather_briefing/cli.py[414-437]
weather_briefing/weather_context.py[245-277]
weather_briefing/weather_context.py[745-774]
weather_briefing/capabilities.py[39-50]

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_PROVIDER_METADATA` declares QWeather does not support `CapabilityName.ALLERGEN`, but the QWeather adapter can produce allergen-related context (via `allergen_advice_available`), which is surfaced to the rest of the system through `SourceDocument.has_allergen_information`. This makes `ProviderCapabilities.supports(ALLERGEN)` contradict the adapter’s real output.

## Issue Context
`ProviderCapabilities` is documented as describing capabilities “exposed by one provider adapter”. Independently, `QWeatherProvider` already computes `allergen_advice_available`, and `snapshot_to_documents()` maps that into a document flag used elsewhere.

## Fix Focus Areas
- weather_briefing/cli.py[414-437]
- weather_briefing/weather_context.py[245-277]
- weather_briefing/weather_context.py[745-774]
- weather_briefing/capabilities.py[39-50]

## What to change
Choose one consistent contract and implement it:
1) If `CapabilityName.ALLERGEN` means “adapter can supply any allergen information used by the system”, add `CapabilityName.ALLERGEN` to QWeather’s metadata (and update any expectations/tests accordingly).

OR

2) If `CapabilityName.ALLERGEN` is intended to mean only the dedicated/structured allergen capability (e.g., an `AllergenSnapshot`-style output), then adjust naming/docs/metadata so it can’t be misread as “no allergen info”, and consider whether `allergen_advice_available` / `has_allergen_information` should remain the authoritative signal for allergen advice availability.

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

---

Previous review results

Review updated until commit ecf4e29

Results up to commit 9a3b918 ⚖️ Balanced

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

1. CapabilityProviderSet missing notes entry ✓ Resolved 📘 Rule violation ⌂ Architecture

Description

The PR introduces a non-obvious architectural change (capability composition via
CapabilityProviderSet) but does not add a corresponding rationale/trade-offs/boundaries entry in
docs/notes.md. This leaves the new extension boundary undocumented in the repository’s designated
architecture notes.

Code

weather_briefing/capabilities.py[R52-86]

+@dataclass(frozen=True, slots=True)
+class CapabilityProviderSet:
+    """Compose independent weather and air-quality capabilities."""
+
+    weather: ContextCapabilityProvider
+    weather_metadata: ProviderCapabilities
+    air_quality: AirQualityProvider | None = None
+    air_quality_metadata: ProviderCapabilities | None = None
+
+    async def fetch(
+        self,
+        latitude: float,
+        longitude: float,
+        *,
+        forecast_date: pendulum.Date | None = None,
+    ) -> WeatherContextSnapshot:
+        """Fetch weather and fill a missing current air-quality capability."""
+        snapshot = await _fetch_context(self.weather, latitude, longitude, forecast_date)
+        if snapshot.air_quality is not None or forecast_date is not None:
+            return snapshot
+        if self.air_quality is None:
+            from .weather_context import WeatherContextError
+
+            raise WeatherContextError("Weather source did not provide air quality; configure AQICN_API_TOKEN")
+        try:
+            air_quality = await self.air_quality.fetch(
+                latitude,
+                longitude,
+                datetime_timezone_specifier(snapshot.observed_at, context="Weather snapshot time"),
+            )
+        except AirQualityError:
+            from .weather_context import WeatherContextError
+
+            raise WeatherContextError("Weather source did not provide air quality and AQICN fallback failed") from None
+        return replace(snapshot, air_quality=air_quality)

Relevance

⭐⭐⭐ High

docs/notes.md is actively used for non-obvious architecture rationale; prior PRs added/expanded
notes entries for boundaries/trade-offs.

PR-#37
PR-#54
PR-#58

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

CapabilityProviderSet is introduced as a new composition/extension boundary in code, while
docs/notes.md (the designated place for non-obvious architectural decisions) contains no matching
entry documenting this decision’s rationale, trade-offs, or operating boundaries. This violates the
requirement to document such decisions in docs/notes.md.

Rule 2141673: Document non-obvious architectural decisions in docs/notes.md
weather_briefing/capabilities.py[52-86]
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
A new capability composition boundary (`CapabilityProviderSet`) is introduced, but `docs/notes.md` does not include an entry documenting the decision, rationale, trade-offs, and operating boundaries/assumptions.

## Issue Context
`docs/notes.md` is explicitly intended to capture key architectural decisions whose rationale is not obvious from the external contract. Capability composition affects how providers are extended and how missing capabilities (like air quality) are supplemented.

## Fix Focus Areas
- weather_briefing/capabilities.py[52-96]
- weather_briefing/cli.py[415-463]
- docs/notes.md[1-50]

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

---

2. AirQualitySupplementingWeatherProvider doc is stale ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

docs/design.md still describes AirQualitySupplementingWeatherProvider as the current air-quality
supplementation mechanism, but the CLI now wires weather context through CapabilityProviderSet.
This makes the design doc describe a superseded contract as current, which can mislead maintainers
and implementers.

Code

docs/design.md[R51-52]

+能力组合边界由 `capabilities.py` 的 `CapabilityProviderSet` 承担。天气、空气质量、过敏原、生活指数、预警和短时预报属于可独立声明的 capability；现有 QWeather/Open-Meteo 完整上下文 adapter 暂时挂在天气槽位，AQICN 挂在空气质量槽位。这样本地气象机构可以只实现预警或 nowcast，而不必伪装为完整天气 provider；后续能力 provider 不应为填充无关字段而发起额外请求。
+

Relevance

⭐⭐⭐ High

Team previously accepted doc-drift fixes in docs (avoid duplication/drift) and frequently updates
design.md with refactors.

PR-#62
PR-#63
PR-#59

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The documentation describes AirQualitySupplementingWeatherProvider as the current mechanism, while
the updated CLI composition constructs and returns CapabilityProviderSet instead. This violates
the requirement that modified docs under docs/ must not present superseded designs as current
without clearly marking them as historical/deprecated.

Rule 2141667: Docs in docs/ must not describe superseded contracts as current
docs/design.md[49-60]
weather_briefing/cli.py[415-463]

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` currently documents `AirQualitySupplementingWeatherProvider` as the active air-quality supplementation path, but the implementation wiring in `weather_briefing/cli.py` now uses `CapabilityProviderSet`.

## Issue Context
This PR introduces capability composition and updates the runtime composition root accordingly. The design doc should describe the current contract, and any legacy approach must be explicitly marked as historical/deprecated.

## Fix Focus Areas
- docs/design.md[49-60]
- weather_briefing/cli.py[415-463]

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

---

3. Wrong capability metadata ✓ Resolved 🐞 Bug ≡ Correctness

Description

_weather_context_provider() hard-codes weather_metadata.capabilities to include
LIFESTYLE/AIR_QUALITY/ALLERGEN even though the configured underlying weather adapters do not
consistently populate those normalized fields, so ProviderCapabilities.supports() can return
incorrect results. This violates the contract implied by ProviderCapabilities (“capabilities
exposed by one provider adapter”) and can mislead any capability-based routing/validation that uses
this metadata.

Code

weather_briefing/cli.py[R440-450]

+        weather_metadata=ProviderCapabilities(
+            provider_id="weather-composite",
+            provider_name="Weather provider composite",
+            capabilities=frozenset(
+                {
+                    CapabilityName.WEATHER,
+                    CapabilityName.LIFESTYLE,
+                    CapabilityName.AIR_QUALITY,
+                    CapabilityName.ALLERGEN,
+                }
+            ),

Relevance

⭐⭐ Medium

Team enforces correctness/contract alignment (accepted similar contract-metadata/type fixes), but no
direct history on ProviderCapabilities accuracy.

PR-#24
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The CLI declares broad capabilities on weather_metadata, but provider implementations show those
fields are not consistently provided in the normalized snapshot: Open-Meteo does not set
lifestyle_advice, and QWeather does not set the normalized allergen field. This makes
ProviderCapabilities.supports() an unreliable indicator of actual support.

weather_briefing/cli.py[415-462]
weather_briefing/capabilities.py[39-50]
weather_briefing/weather_context.py[312-321]
weather_briefing/weather_context.py[459-472]
weather_briefing/models.py[152-165]

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_context_provider()` currently constructs `ProviderCapabilities` for the composed weather provider with a hard-coded capability set that does not match what the selected provider(s) actually expose via the normalized `WeatherContextSnapshot` fields.

## Issue Context
- `ProviderCapabilities` is documented as describing capabilities exposed by one provider adapter.
- Open-Meteo snapshots do not populate `lifestyle_advice` at all (it stays the default empty tuple), so advertising `CapabilityName.LIFESTYLE` on the weather adapter is incorrect when Open-Meteo is the chosen weather provider.
- QWeather snapshots do not populate the normalized `allergen` snapshot field (only `allergen_advice_available`), so advertising `CapabilityName.ALLERGEN` as a weather adapter capability is incorrect.

## Fix Focus Areas
- weather_briefing/cli.py[415-462]
- weather_briefing/capabilities.py[39-60]

## Suggested approach
1. Replace the hard-coded `capabilities=frozenset({...})` with capabilities derived from the actual configured weather provider(s) (e.g., mapping from provider name/type to capability set), or
2. If the intent is to describe aggregate capabilities of the entire `CapabilityProviderSet`, introduce a separate metadata record for the composed set (e.g., `composed_metadata`) and keep `weather_metadata` limited to the weather adapter’s real capabilities.
3. Ensure `CapabilityName` entries correspond to the normalized snapshot fields that are actually populated (e.g., only claim `ALLERGEN` when `snapshot.allergen` is supported/populated).

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

---

Results up to commit a7440df ⚖️ Balanced

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

1. Dated dispatch may TypeError ✓ Resolved 🐞 Bug ☼ Reliability

Description

_fetch_context() uses getattr(provider, "fetch_for_date") and then blindly awaits it, so a provider
with a non-callable or incompatible fetch_for_date attribute will raise a TypeError instead of the
expected WeatherContextError.
This diverges from the existing fetch_weather_context() contract which gates dated support via
DatedWeatherContextProvider and consistently raises WeatherContextError when unsupported.

Code

weather_briefing/capabilities.py[R98-112]

+async def _fetch_context(
+    provider: ContextCapabilityProvider,
+    latitude: float,
+    longitude: float,
+    forecast_date: pendulum.Date | None,
+) -> WeatherContextSnapshot:
+    """Call providers that support either current or dated context."""
+    if forecast_date is None:
+        return await provider.fetch(latitude, longitude)
+    fetch_for_date = getattr(provider, "fetch_for_date", None)
+    if fetch_for_date is None:
+        from .weather_context import WeatherContextError
+
+        raise WeatherContextError(f"{type(provider).__name__} does not support target forecast dates")
+    return await fetch_for_date(latitude, longitude, forecast_date)

Relevance

⭐⭐⭐ High

Team wraps provider contract failures into WeatherContextError rather than leaking
TypeError/KeyError (PR #35, #37).

PR-#35
PR-#37
PR-#50

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

_fetch_context() only checks for presence of fetch_for_date and then calls it, which can raise
TypeError if the attribute exists but is not a compatible async callable. The existing
weather-context boundary uses DatedWeatherContextProvider + isinstance() to ensure unsupported
providers fail with WeatherContextError instead of leaking unrelated exception types.

weather_briefing/capabilities.py[98-112]
weather_briefing/weather_context.py[59-71]
weather_briefing/weather_context.py[665-676]

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.capabilities._fetch_context()` detects dated support via `getattr(provider, "fetch_for_date", None)` and then awaits the result without validating it is callable / conforms to the dated-provider contract. This can leak a raw `TypeError` (or other unexpected exception) instead of raising the domain-specific `WeatherContextError` used elsewhere.

### Issue Context
The codebase already has a dated-provider boundary defined in `weather_briefing.weather_context` via `DatedWeatherContextProvider` and `fetch_weather_context()`, which raises `WeatherContextError` when dated fetch is not supported. Aligning `_fetch_context()` to that contract avoids inconsistent behavior and prevents unexpected exception types.

### Fix Focus Areas
- weather_briefing/capabilities.py[98-112]

### Suggested fix
In `_fetch_context()`, replace the `getattr()` approach with the existing contract check:
- lazily import `DatedWeatherContextProvider` + `WeatherContextError` (to keep import boundaries similar to current code)
- `if not isinstance(provider, DatedWeatherContextProvider): raise WeatherContextError(...)`
- otherwise `return await provider.fetch_for_date(latitude, longitude, forecast_date)`

(Alternative minimal fix if you want to keep duck-typing: check `callable(fetch_for_date)` before calling, and raise `WeatherContextError` if not callable.)

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

---

Results up to commit 706f574 ⚖️ Balanced

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

1. Metadata registry can drift ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

_weather_provider_metadata() directly indexes _WEATHER_PROVIDER_METADATA for every active provider
name, so adding a new weather provider without also updating this parallel metadata registry will
crash provider construction with KeyError. This creates a fragile extension point precisely where
the PR is introducing provider composition as an extensibility boundary.

Code

weather_briefing/cli.py[R440-452]

+def _weather_provider_metadata(names: Sequence[str]) -> ProviderCapabilities:
+    """Describe capabilities common to every active fallback provider."""
+    metadata = [_WEATHER_PROVIDER_METADATA[name] for name in names]
+    if len(metadata) == 1:
+        return metadata[0]
+    capabilities = metadata[0].capabilities
+    for item in metadata[1:]:
+        capabilities &= item.capabilities
+    return ProviderCapabilities(
+        provider_id="weather-composite",
+        provider_name="Weather provider composite",
+        capabilities=capabilities,
+    )

Relevance

⭐⭐⭐ High

Team previously fixed KeyError crashes via explicit contract validation/errors (PR35, PR52); likely
accept drift-proofing metadata lookup.

PR-#35
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

_weather_provider_metadata() uses direct dict indexing for each provider name, so any missing
entry will raise KeyError. Provider names are validated against the supported provider set, but
the metadata registry is a separate structure that must be kept in sync when adding/renaming
providers, otherwise the CLI crashes during provider assembly.

weather_briefing/cli.py[414-452]
weather_briefing/config.py[28-29]
weather_briefing/config.py[183-193]
weather_briefing/registries.py[6-11]

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_provider_metadata()` assumes every active provider id exists in `_WEATHER_PROVIDER_METADATA` and performs direct indexing. If a new weather provider is added to the supported/constructed provider set but metadata isn’t updated, CLI construction will raise `KeyError` instead of producing a clear configuration/developer error.

## Issue Context
There are now two parallel registries in `cli.py`: one for construction (`WEATHER_PROVIDER_BUILDERS`) and one for capability metadata (`_WEATHER_PROVIDER_METADATA`). They can drift independently.

## Fix Focus Areas
- weather_briefing/cli.py[414-452]

## Suggested fix
1. Make `_weather_provider_metadata()` robust:
  - Use `_WEATHER_PROVIDER_METADATA.get(name)` and raise a `ValueError` (or `ConfigurationError`) with a clear message if missing.
2. Add an internal consistency check at import time (or in a small helper/test):
  - Assert that `set(WEATHER_PROVIDER_BUILDERS)` (or `SUPPORTED_WEATHER_PROVIDERS`) is a subset of `_WEATHER_PROVIDER_METADATA` keys, so drift is caught immediately during development.
3. (Optional) Add a unit test that would fail if a supported provider is missing metadata.

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

---

Results up to commit ecf4e29 ⚖️ Balanced

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

1. QWeather allergen metadata mismatch 🐞 Bug ≡ Correctness

Description

_WEATHER_PROVIDER_METADATA marks QWeather as not supporting CapabilityName.ALLERGEN, but
QWeatherProvider can set WeatherContextSnapshot.allergen_advice_available and
snapshot_to_documents() exposes that as SourceDocument.has_allergen_information. This makes
ProviderCapabilities.supports(ALLERGEN) inconsistent with what the QWeather adapter can actually
emit, so supports() cannot be trusted to reflect allergen info availability for QWeather.

Code

weather_briefing/cli.py[R414-425]

+_WEATHER_PROVIDER_METADATA: dict[str, ProviderCapabilities] = {
+    WeatherProviderName.QWEATHER: ProviderCapabilities(
+        provider_id=WeatherProviderName.QWEATHER,
+        provider_name="QWeather",
+        capabilities=frozenset(
+            {
+                CapabilityName.WEATHER,
+                CapabilityName.AIR_QUALITY,
+                CapabilityName.LIFESTYLE,
+            }
+        ),
+    ),

Relevance

⭐ Low

Team treats QWeather allergen as lifestyle advice; tests/docs enforce conservative capabilities. See
allergen work in PRs 23/31.

PR-#23
PR-#31

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The CLI metadata omits ALLERGEN for QWeather, but the existing QWeather adapter computes
allergen_advice_available from the indices response and snapshot_to_documents() turns that into
has_allergen_information on the produced SourceDocument. This demonstrates QWeather can expose
allergen-related context even though the new supports(ALLERGEN) metadata reports otherwise.

weather_briefing/cli.py[414-437]
weather_briefing/weather_context.py[245-277]
weather_briefing/weather_context.py[745-774]
weather_briefing/capabilities.py[39-50]

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_PROVIDER_METADATA` declares QWeather does not support `CapabilityName.ALLERGEN`, but the QWeather adapter can produce allergen-related context (via `allergen_advice_available`), which is surfaced to the rest of the system through `SourceDocument.has_allergen_information`. This makes `ProviderCapabilities.supports(ALLERGEN)` contradict the adapter’s real output.

## Issue Context
`ProviderCapabilities` is documented as describing capabilities “exposed by one provider adapter”. Independently, `QWeatherProvider` already computes `allergen_advice_available`, and `snapshot_to_documents()` maps that into a document flag used elsewhere.

## Fix Focus Areas
- weather_briefing/cli.py[414-437]
- weather_briefing/weather_context.py[245-277]
- weather_briefing/weather_context.py[745-774]
- weather_briefing/capabilities.py[39-50]

## What to change
Choose one consistent contract and implement it:
1) If `CapabilityName.ALLERGEN` means “adapter can supply any allergen information used by the system”, add `CapabilityName.ALLERGEN` to QWeather’s metadata (and update any expectations/tests accordingly).

OR

2) If `CapabilityName.ALLERGEN` is intended to mean only the dedicated/structured allergen capability (e.g., an `AllergenSnapshot`-style output), then adjust naming/docs/metadata so it can’t be misread as “no allergen info”, and consider whether `allergen_advice_available` / `has_allergen_information` should remain the authoritative signal for allergen advice availability.

ⓘ 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 a7440df

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 706f574

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit ecf4e29

[qodo-code-review]

PR Summary by Qodo

Refactor weather providers into composable capability sets

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Introduce a capability composition layer for weather and optional air-quality supplementation.
• Expose provider capability metadata to prevent unsupported features from being advertised.
• Add tests covering supplementation, dated-context behavior, and metadata intersection semantics.

Diagram

graph TD
  cli["cli.py (wiring)"] --> capset["CapabilityProviderSet"] --> snap["WeatherContextSnapshot"]
  capset --> weather["Weather provider (fallback)"] --> q{{"QWeather API"}}
  weather --> o{{"Open-Meteo API"}}
  capset --> aq{{"AQICN API"}}

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep a dedicated AirQualitySupplementingWeatherProvider wrapper

• ➕ Minimizes surface area by only addressing AQ supplementation
• ➕ Avoids introducing capability metadata concepts until needed
• ➖ Doesn’t generalize to alerts/nowcast/allergen/lifestyle composition
• ➖ Harder to evolve into multi-capability routing without more wrappers

2. Fully split vendor adapters into per-capability providers now

• ➕ Cleanest capability boundaries and independent substitution
• ➕ Enables true mix-and-match across vendors per capability
• ➖ Likely duplicates network calls to the same vendor, increasing latency/quota usage
• ➖ More complex partial-failure handling and consistency across capability snapshots

3. Advertise union-of-capabilities for fallbacks ("at least one supports")

• ➕ More optimistic feature discovery (e.g., allergen available sometimes)
• ➕ Could enable UI/UX that adapts to the selected runtime provider
• ➖ Breaks the current implied contract of supports() being a guarantee
• ➖ Requires new metadata semantics and downstream handling to avoid false claims

Recommendation: The staged approach in this PR is the best near-term tradeoff: introduce a single composition boundary (CapabilityProviderSet) while keeping existing full-context vendor adapters intact to avoid duplicated requests. The choice to compute fallback metadata as the intersection of active providers is also sound because it preserves supports() as a guarantee. If future requirements need "sometimes available" capabilities, add a separate aggregate metadata type rather than changing supports() semantics.

Files changed (6) +370 / -8

Refactor (2) +177 / -5

capabilities.pyIntroduce CapabilityProviderSet and capability metadata model +114/-0

Introduce CapabilityProviderSet and capability metadata model

• Adds CapabilityName enum, ProviderCapabilities metadata (with supports()), and CapabilityProviderSet to compose a weather provider with optional air-quality supplementation. Implements dated-context dispatch with explicit support checks and converts AQ fallback failures into WeatherContextError with actionable messaging.

weather_briefing/capabilities.py

cli.pyWire CLI to build capability composition and compute fallback metadata +63/-5

Wire CLI to build capability composition and compute fallback metadata

• Replaces the previous AQ supplementation wrapper with CapabilityProviderSet, passing through the weather provider (single or fallback) plus optional AQICN provider. Adds a provider capability metadata registry and computes composite metadata as the intersection of active providers’ capabilities, preventing unsupported features from being advertised.

weather_briefing/cli.py

Tests (2) +188 / -2

test_capabilities.pyAdd unit tests for capability composition and supplementation rules +151/-0

Add unit tests for capability composition and supplementation rules

• Introduces tests for ProviderCapabilities.supports(), current-context air-quality supplementation via AQICN, and ensuring dated (target-date) contexts do not use current AQ supplementation. Adds negative tests for missing AQ configuration, wrapped AQ provider failures, and enforcing dated-provider support checks.

tests/test_capabilities.py

test_cli.pyUpdate CLI tests for capability metadata and composite intersections +37/-2

Update CLI tests for capability metadata and composite intersections

• Adjusts provider-selection assertions to validate weather_metadata contents (including capability support) rather than only non-null provider objects. Adds coverage ensuring composite fallback metadata is the intersection of candidate capabilities and validates rejection of unregistered providers in the metadata registry.

tests/test_cli.py

Documentation (2) +5 / -1

design.mdDocument capability composition boundary and provider slotting +3/-1

Document capability composition boundary and provider slotting

• Adds design notes describing CapabilityProviderSet as the composition boundary and explains how existing full-context adapters map into capability slots (weather vs air quality supplementation). Updates terminology from the prior AQ supplementation wrapper to the new composition type.

docs/design.md

notes.mdClarify staged capability modeling and fallback metadata semantics +2/-0

Clarify staged capability modeling and fallback metadata semantics

• Explains why current vendor adapters remain full-context to avoid duplicate requests and partial-failure complexity. Documents that fallback metadata only claims capabilities common to all candidates, preserving supports() as a guarantee.

docs/notes.md

[qodo-code-review]

1. Duplicated dated dispatch 🐞 Bug ⚙ Maintainability

CapabilityProviderSet introduces _fetch_context() that re-implements the same “current vs
forecast_date” dispatch already provided by weather_context.fetch_weather_context(), creating two
sources of truth that can drift in behavior and error semantics. This is especially risky because
both paths are exercised in production (service.py uses fetch_weather_context(), while
CapabilityProviderSet uses _fetch_context()).

Agent Prompt

### Issue description
`weather_briefing/capabilities.py` defines `_fetch_context()` to route between `fetch()` and `fetch_for_date()`, but the repo already has `weather_briefing/weather_context.py:fetch_weather_context()` that does the same routing. Keeping both increases the chance of future drift (e.g., one adds new validation or different error messages while the other doesn’t).

### Issue Context
- `CapabilityProviderSet.fetch()` currently calls `_fetch_context()`.
- `BriefingService` still calls `fetch_weather_context()` directly.

### Fix Focus Areas
- Prefer a single shared dispatch implementation (either call `fetch_weather_context()` from `capabilities.py`, or move dispatch into a shared helper used by both).
- If the callable-guard behavior is desired, consider adding it to the shared helper so both code paths behave consistently.

- weather_briefing/capabilities.py[61-114]
- weather_briefing/weather_context.py[665-676]
- weather_briefing/service.py[331-338]

ⓘ 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 ecf4e29

[coderabbitai]

🧹 Nitpick comments (3)

weather_briefing/cli.py (2)

477-480: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Redundant recomputation of the already-built active_names filter for logging.

active_names (built in the loop above) already contains exactly the providers for which name != WeatherProviderName.QWEATHER or _qweather_is_configured(settings). This generator recomputes the identical condition instead of reusing active_names, risking future drift if the filter logic changes in only one place.

♻️ Reuse active_names instead of recomputing the filter

     _LOGGER.info(
         "Weather provider order providers=%s",
-        ",".join(name for name in names if name != WeatherProviderName.QWEATHER or _qweather_is_configured(settings)),
+        ",".join(active_names),
     )

🤖 Prompt for AI Agents

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

In `@weather_briefing/cli.py` around lines 477 - 480, Update the Weather provider
order log in the surrounding flow to reuse the already-built active_names
collection instead of regenerating and refiltering names. Preserve the existing
comma-separated logging format and the active_names ordering.

---

414-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Provider-capability classification table is a Python constant.

_WEATHER_PROVIDER_METADATA maps provider name → capability set — a classification table. The repo already has provider_defaults.json plus reference_string_tuple(...) infrastructure used for comparable reference data (e.g. weather_provider_order, qweather_lifestyle_index_types in config.py). Consider moving this mapping into that same JSON file for consistency, since adding/adjusting a provider's declared capabilities would then not require a code change.

As per coding guidelines, "Keep domain reference data such as geographic bounds, classification tables, and matching patterns in validated data files rather than Python constants."

🤖 Prompt for AI Agents

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

In `@weather_briefing/cli.py` around lines 414 - 437, Move the
provider-to-capabilities classification from _WEATHER_PROVIDER_METADATA in
weather_briefing/cli.py into provider_defaults.json, using the existing
reference_string_tuple(...) configuration infrastructure and validation
patterns. Update the metadata-loading logic to read the JSON-backed values while
preserving the current ProviderCapabilities structure and provider behavior.

Source: Coding guidelines

weather_briefing/capabilities.py (1)

61-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Core capability layer hardcodes a specific vendor's env var name.

self.air_quality is typed as the generic AirQualityProvider | None, but the raised WeatherContextError messages at lines 75 and 85 hardcode "AQICN_API_TOKEN"/"AQICN fallback". If a non-AQICN air-quality provider is ever plugged into this slot, the error text becomes misleading. As per coding guidelines, **/*.py should "keep core data platform-neutral and place vendor or delivery syntax in adapters," so this vendor-specific wording belongs at the CLI/adapter boundary (e.g., surfaced via air_quality_metadata/config validation) rather than baked into CapabilityProviderSet.

♻️ Example: keep the core message provider-neutral

-            raise WeatherContextError("Weather source did not provide air quality; configure AQICN_API_TOKEN")
+            raise WeatherContextError("Weather source did not provide air quality and no air-quality provider is configured")

-            raise WeatherContextError("Weather source did not provide air quality and AQICN fallback failed") from None
+            raise WeatherContextError("Weather source did not provide air quality and the configured air-quality provider failed") from None

🤖 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/capabilities.py` around lines 61 - 86, Update
CapabilityProviderSet.fetch to remove AQICN-specific wording from both
WeatherContextError messages. Use provider-neutral descriptions for missing or
failed air-quality fallback, and leave vendor-specific token/configuration
guidance to the CLI or adapter validation layer.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@weather_briefing/capabilities.py`:
- Around line 61-86: Update CapabilityProviderSet.fetch to remove AQICN-specific
wording from both WeatherContextError messages. Use provider-neutral
descriptions for missing or failed air-quality fallback, and leave
vendor-specific token/configuration guidance to the CLI or adapter validation
layer.

In `@weather_briefing/cli.py`:
- Around line 477-480: Update the Weather provider order log in the surrounding
flow to reuse the already-built active_names collection instead of regenerating
and refiltering names. Preserve the existing comma-separated logging format and
the active_names ordering.
- Around line 414-437: Move the provider-to-capabilities classification from
_WEATHER_PROVIDER_METADATA in weather_briefing/cli.py into
provider_defaults.json, using the existing reference_string_tuple(...)
configuration infrastructure and validation patterns. Update the
metadata-loading logic to read the JSON-backed values while preserving the
current ProviderCapabilities structure and provider behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c645762b-4342-431b-9d4c-5a134ea432e3

📥 Commits

Reviewing files that changed from the base of the PR and between ebdb5a8 and ecf4e29.

📒 Files selected for processing (6)

• docs/design.md
• docs/notes.md
• tests/test_capabilities.py
• tests/test_cli.py
• weather_briefing/capabilities.py
• weather_briefing/cli.py