Add configurable fallback LLM provider

[IceCodeNew]

Summary

• add paired LLM_FALLBACK_PROVIDER and LLM_FALLBACK_MODEL settings
• retry primary request failures once across briefing, notification-decision, and status-translation operations
• keep output-contract failures on the existing bounded repair path and close both provider resources

Verification

• prek run --all-files
• uv run --frozen --with pytest --with pytest-cov -- pytest --cov --cov-branch --cov-report=xml (1159 passed; 99% coverage, no regression from master)

Summary by CodeRabbit

• New Features

  • Added optional fallback LLM configuration through environment settings.
  • Automatically retries requests with the fallback model when the primary model encounters a request failure.
  • Keeps using the fallback model for the remainder of the current session after switching.
  • Added support for fallback-specific API credentials and endpoints.

• Documentation

  • Documented configuration requirements, fallback behavior, and error-handling rules.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.85%. Comparing base (1e033ae) to head (55602fa).
⚠️ Report is 2 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff            @@
##           master     #115    +/-   ##
========================================
  Coverage   99.85%   99.85%            
========================================
  Files         109      111     +2     
  Lines       11647    11973   +326     
  Branches      708      717     +9     
========================================
+ Hits        11630    11956   +326     
  Misses         12       12            
  Partials        5        5            

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

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7fd8d98d-8bff-48d9-a625-0e31da59ab0c

📥 Commits

Reviewing files that changed from the base of the PR and between 77a2a24 and 55602fa.

📒 Files selected for processing (4)

• docs/design.md
• env.example
• tests/test_config.py
• weather_briefing/config/settings.py

---

📝 Walkthrough

Walkthrough

Adds optional LLM fallback configuration, validates paired provider settings, composes a sticky fallback wrapper, preserves request and cancellation error semantics, closes both providers safely, and documents and tests the behavior.

Changes

LLM fallback

Layer / File(s)	Summary
Fallback configuration and validation weather_briefing/config/settings.py, tests/test_config.py, env.example	Adds nullable fallback settings, paired-variable validation, completion-capability checks, fallback-only override validation, and environment examples.
Provider composition and contract weather_briefing/llm/fallback.py, weather_briefing/llm/__init__.py, weather_briefing/composition/providers.py, tests/test_cli.py	Defines and exports the complete-provider contract, constructs primary and optional fallback adapters, and forwards normalized fallback connection settings.
Sticky fallback routing weather_briefing/llm/fallback.py, tests/test_llm_fallback.py, docs/notes.md, docs/requirements.md, docs/design.md	Switches permanently to the fallback after the first LLMRequestError, leaves validation errors unchanged, preserves error context, and logs redacted diagnostics.
Provider resource cleanup weather_briefing/llm/fallback.py, tests/test_llm_fallback.py	Closes both providers, aggregates ordinary cleanup failures, and preserves cancellation precedence.

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

Sequence Diagram(s)

sequenceDiagram
  participant Application
  participant FallbackLLMProvider
  participant PrimaryLLM
  participant FallbackLLM
  Application->>FallbackLLMProvider: invoke operation
  FallbackLLMProvider->>PrimaryLLM: send request
  PrimaryLLM-->>FallbackLLMProvider: LLMRequestError
  FallbackLLMProvider->>FallbackLLM: retry operation
  FallbackLLM-->>Application: return result
  Application->>FallbackLLMProvider: close provider
  FallbackLLMProvider->>PrimaryLLM: aclose()
  FallbackLLMProvider->>FallbackLLM: aclose()

Loading

Possibly related PRs

• IceCodeNew/weather-briefing#57: Introduces the LLMRequestError distinction used by fallback routing.
• IceCodeNew/weather-briefing#96: Introduces shared LLM provider contracts and request-error handling used by this wrapper.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 30.23% 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
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: adding a configurable fallback LLM provider.

✨ 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/configurable-llm-fallback

---

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

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 46 rules

1. Repair switches LLM provider ✓ Resolved 🐞 Bug ≡ Correctness

Description

FallbackLLMProvider always tries the primary first on every summarize() call, so
summarize_validated() repair retries after a fallback response can switch back to the primary
provider. This violates the documented requirement that structured-output/validation failures be
repaired on the provider that produced them and can hide invalid output by succeeding via a
different provider.

Code

weather_briefing/llm/fallback.py[R58-81]

+    async def _request(
+        self,
+        operation: str,
+        primary_call: Callable[[], Awaitable[_Result]],
+        fallback_call: Callable[[], Awaitable[_Result]],
+    ) -> _Result:
+        try:
+            return await primary_call()
+        except LLMRequestError:
+            _LOGGER.warning(
+                "Primary LLM request failed; trying fallback operation=%s primary=%s fallback=%s",
+                operation,
+                self._primary_name,
+                self._fallback_name,
+            )
+            return await fallback_call()
+
+    async def summarize(self, system_prompt: str, payload: dict[str, object]) -> dict[str, object]:
+        """Generate a briefing, falling back only after a request failure."""
+        return await self._request(
+            "summarize",
+            lambda: self._primary.summarize(system_prompt, payload),
+            lambda: self._fallback.summarize(system_prompt, payload),
+        )

Relevance

⭐⭐⭐ High

Docs require contract failures repaired on same provider; current fallback can switch on retry,
hiding invalid output.

PR-#101

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

summarize_validated() retries by calling provider.summarize() again after LLMError, but
FallbackLLMProvider has no per-operation pinning and always starts with the primary on each call;
this contradicts the newly documented rule that contract failures remain with the provider that
produced them.

weather_briefing/application/summarization.py[19-61]
weather_briefing/llm/fallback.py[58-81]
README.md[131-132]
docs/design.md[184-185]
docs/requirements.md[66-68]

Agent prompt

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

### Issue description
When the primary request fails and the fallback is used, later retries triggered by output-contract/validation failures (LLMError) can re-enter `FallbackLLMProvider.summarize()` and start with the primary again. This causes provider switching during the bounded repair loop, contradicting the documented behavior (“contract failures stay with the provider that returned them”) and the requirement to not mask invalid outputs via provider switching.

### Issue Context
- `summarize_validated()` performs multiple `provider.summarize(...)` calls across attempts.
- `FallbackLLMProvider` is stateless per operation and always prefers the primary on each call.

### Fix Focus Areas
- weather_briefing/llm/fallback.py[58-102]
- weather_briefing/application/summarization.py[19-61]
- README.md[131-132]
- docs/design.md[184-185]
- docs/requirements.md[67-67]

### Implementation direction
- Ensure the provider used for a given logical summarization operation is *pinned* once a provider successfully returns a response (even if that response later fails validation).
- One approach: add a request/operation-scoped pinned wrapper (e.g., `FallbackLLMProvider.pinned()` returning an object that remembers which provider succeeded first and uses it for subsequent `summarize()` calls), and update `summarize_validated()` to use the pinned wrapper for all attempts within the function.
- Add a regression test that simulates: primary `LLMRequestError` -> fallback returns an invalid response causing `LLMError` -> next repair attempt must call fallback again (not primary).

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

---

2. Fallback env test isolation ✓ Resolved 🐞 Bug ☼ Reliability

Description

test_llm_fallback_connection_settings_require_fallback assumes
LLM_FALLBACK_PROVIDER/LLM_FALLBACK_MODEL are unset but never clears them, so an ambient
developer/CI environment that exports both will bypass the expected ConfigurationError and make
this test fail nondeterministically.

Code

tests/test_config.py[R1085-1091]

+@pytest.mark.parametrize("name", ("LLM_FALLBACK_API_KEY", "LLM_FALLBACK_BASE_URL"))
+def test_llm_fallback_connection_settings_require_fallback(monkeypatch, name: str) -> None:
+    _required_environment(monkeypatch)
+    monkeypatch.setenv(name, "configured")
+
+    with pytest.raises(ConfigurationError, match=rf"{name} requires LLM_FALLBACK_PROVIDER and LLM_FALLBACK_MODEL"):
+        Settings.from_env()

Relevance

⭐⭐⭐ High

Team previously accepted clearing env vars in tests to avoid ambient CI/dev flakiness.

PR-#108

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test sets only the connection variable and expects an error that is only triggered when fallback
provider/model are missing; the helper used by the test does not clear those fallback variables, so
inherited env values can change the code path and break the expectation.

tests/test_config.py[19-30]
tests/test_config.py[1085-1091]
weather_briefing/config/settings.py[165-176]

Agent prompt

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

## Issue description
`test_llm_fallback_connection_settings_require_fallback` depends on `LLM_FALLBACK_PROVIDER` and `LLM_FALLBACK_MODEL` being absent, but the test only sets `LLM_FALLBACK_API_KEY` or `LLM_FALLBACK_BASE_URL`. If the test process inherits `LLM_FALLBACK_PROVIDER`+`LLM_FALLBACK_MODEL` from the environment, `Settings.from_env()` won’t raise and the test will fail.

## Issue Context
`_required_environment()` sets required variables but does not sanitize unrelated ones, so tests should explicitly `delenv()` optional variables they rely on being unset.

## Fix Focus Areas
- tests/test_config.py[1085-1091]

## Suggested change
Inside `test_llm_fallback_connection_settings_require_fallback`, add:
- `monkeypatch.delenv("LLM_FALLBACK_PROVIDER", raising=False)`
- `monkeypatch.delenv("LLM_FALLBACK_MODEL", raising=False)`

before `monkeypatch.setenv(name, "configured")` (or alternatively extend `_required_environment()` to clear all `LLM_FALLBACK_*` vars by default).

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

---

3. Fallback LLM default undocumented ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

LLM_FALLBACK_PROVIDER/LLM_FALLBACK_MODEL are introduced as new configurable behavior, but the
user-facing docs do not state the single global default (fallback disabled when unset). Without an
explicit default, different docs/examples can drift and operators may misconfigure expected runtime
behavior.

Code

README.md[118]

+- optionally, both `LLM_FALLBACK_PROVIDER` and `LLM_FALLBACK_MODEL`;

Relevance

⭐⭐⭐ High

Team commonly updates docs to clarify unset/empty defaults for env-driven behavior to avoid
drift/misconfig.

PR-#114
PR-#110

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist requires a single documented default for new configuration-controlled implicit
behavior. The README introduces the new optional fallback variables but does not state the default
behavior when they are unset, while Settings.from_env() clearly defaults both fallback values to
None (disabled) unless configured.

Rule 2225203: Document and apply a single global default for configurable implicit behaviors
README.md[113-132]
weather_briefing/config/settings.py[153-176]

Agent prompt

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

## Issue description
New configuration options `LLM_FALLBACK_PROVIDER` / `LLM_FALLBACK_MODEL` are documented as optional, but the docs do not explicitly state the single global default behavior when they are omitted.

## Issue Context
The implementation defaults both settings to `None` (fallback disabled) unless both env vars are set. Compliance requires a clearly documented single default for newly configurable implicit behaviors.

## Fix Focus Areas
- README.md[115-132]
- env.example[9-11]
- weather_briefing/config/settings.py[165-176]

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

---

4. Primary failure log too vague ✓ Resolved 🐞 Bug ◔ Observability

Description

FallbackLLMProvider._request() swallows the primary LLMRequestError when the fallback succeeds and
logs only operation/provider names, so operators lose even a coarse, privacy-safe classification of
the primary failure. This makes primary-provider incident diagnosis harder because the fallback
success path leaves no other trace of what went wrong.

Code

weather_briefing/llm/fallback.py[R68-78]

+        try:
+            return await primary_call()
+        except LLMRequestError:
+            self._using_fallback = True
+            _LOGGER.warning(
+                "Primary LLM request failed; trying fallback operation=%s primary=%s fallback=%s",
+                operation,
+                self._primary_name,
+                self._fallback_name,
+            )
+            return await fallback_call()

Relevance

⭐⭐⭐ High

Repo favors privacy-safe but more informative warning logs for LLM failures
(operation/provider/model); adding error-type fits pattern.

PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

FallbackLLMProvider._request() catches LLMRequestError, flips to fallback, and logs a warning
without any error classification before returning the fallback call; when fallback succeeds, the
primary exception is neither propagated nor otherwise logged. The any-llm adapter raises
LLMRequestError(...) from exc, so the underlying cause type is available in the exception chain
but currently not surfaced in logs on the successful-fallback path.

weather_briefing/llm/fallback.py[60-79]
weather_briefing/llm/any_llm.py[100-111]

Agent prompt

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

### Issue description
When the primary LLM request fails but the fallback succeeds, `FallbackLLMProvider._request()` suppresses the exception and emits a warning without any safe classification (e.g., exception/cause type). That means the only record of the primary failure is a generic warning, which is not enough to triage provider outages while still keeping sensitive exception messages out of logs.

### Issue Context
- `FallbackLLMProvider._request()` catches `LLMRequestError`, logs a generic warning, and immediately returns the fallback result.
- The any-llm adapter wraps provider-specific exceptions as `LLMRequestError(... ) from exc`, so a privacy-safe classification can be derived from `type(exc.__cause__).__name__` (or fallback to `type(exc).__name__`) without logging exception messages.

### Fix Focus Areas
- weather_briefing/llm/fallback.py[60-79]
- weather_briefing/llm/any_llm.py[100-111]

### Suggested change
In the `except LLMRequestError as exc:` block, include a safe field like `primary_error_type=...` (derived from `exc.__cause__` when present) in the warning log message/args, without adding `exc_info` and without logging `str(exc)`.

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

---

View more (8)

5. aclose promise inaccurate ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

FallbackLLMProvider.aclose() claims it will “preserve every cleanup failure,” but in the
CancelledError path it logs fallback cleanup exceptions and then re-raises the primary
cancellation, so the fallback cleanup failure is not preserved/propagated to the caller. This
contradicts the documented behavior that cleanup exceptions are preserved, and can mislead
maintainers about shutdown/cleanup guarantees.

Code

weather_briefing/llm/fallback.py[R109-124]

+    async def aclose(self) -> None:
+        """Close both providers and preserve every cleanup failure."""
+        errors: list[Exception] = []
+        try:
+            await self._primary.aclose()
+        except asyncio.CancelledError:
+            try:
+                await self._fallback.aclose()
+            except asyncio.CancelledError:
+                _LOGGER.warning("Fallback LLM provider cleanup was cancelled while preserving primary cancellation")
+            except Exception as exc:
+                _LOGGER.warning(
+                    "Failed to close fallback LLM provider during cancellation error_type=%s",
+                    type(exc).__name__,
+                )
+            raise

Relevance

⭐⭐⭐ High

Team often fixes doc/behavior mismatches; clarify docstring or behavior for CancelledError cleanup
semantics.

PR-#114
PR-#107

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The aclose() docstring promises to preserve cleanup failures, but the `except
asyncio.CancelledError branch logs fallback cleanup errors and then does a bare raise`, so no
fallback exception can be propagated in that path. The design documentation also states that all
cleanup exceptions are preserved, which doesn’t hold during cancellation.

weather_briefing/llm/fallback.py[109-124]
docs/design.md[182-185]

Agent prompt

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

### Issue description
`FallbackLLMProvider.aclose()` advertises that it “preserve[s] every cleanup failure,” but when `primary.aclose()` raises `asyncio.CancelledError`, the implementation intentionally re-raises the cancellation and only logs any `fallback.aclose()` exception (by type). This means the fallback cleanup failure is **not** preserved/raised, so the docstring and design docs are misleading.

### Issue Context
The current behavior may be intentional (preserve cancellation semantics), but the contract needs to match reality so callers/maintainers don’t assume cleanup failures are always surfaced programmatically.

### Fix Focus Areas
- weather_briefing/llm/fallback.py[109-124]
- docs/design.md[182-185]

### Proposed fix
- Update the `aclose()` docstring to explicitly describe the cancellation behavior, e.g.:
 - cancellation is propagated (original cancellation preserved)
 - fallback cleanup is attempted
 - non-cancellation fallback cleanup failures are logged (type-only) rather than raised
- Update the design doc sentence that says cleanup exceptions are fully preserved to include the cancellation exception policy (or soften the wording similarly).

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

---

6. Fallback masks cancellation ✓ Resolved 🐞 Bug ☼ Reliability

Description

In FallbackLLMProvider.aclose(), when primary.aclose() raises CancelledError, the code awaits
fallback.aclose() but only suppresses Exception; if fallback.aclose() is also cancelled
(raises CancelledError), it can escape and replace the original cancellation instance and
interrupt fallback cleanup.

Code

weather_briefing/llm/fallback.py[R114-122]

+        except asyncio.CancelledError:
+            try:
+                await self._fallback.aclose()
+            except Exception as exc:
+                _LOGGER.warning(
+                    "Failed to close fallback LLM provider during cancellation error_type=%s",
+                    type(exc).__name__,
+                )
+            raise

Relevance

⭐⭐⭐ High

Team previously accepted cancellation-safety fixes to prevent cleanup being skipped/leaking on
CancelledError.

PR-#107

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cancellation branch only catches Exception while closing the fallback provider, so a fallback
CancelledError can escape and replace the original cancellation. The new tests explicitly require
preserving the original cancellation object when fallback close fails, indicating masking is not
desired.

weather_briefing/llm/fallback.py[109-122]
tests/test_llm_fallback.py[217-257]
PR-#107

Agent prompt

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

## Issue description
`FallbackLLMProvider.aclose()` attempts to close the fallback provider when the primary close is cancelled, but it only catches `Exception` during that fallback close attempt. Since `asyncio.CancelledError` inherits from `BaseException` (not `Exception`), cancellation while closing the fallback can propagate and mask the original cancellation.

## Issue Context
Tests assert the primary cancellation instance should be preserved even when fallback close raises a regular exception, but the current implementation still allows fallback `CancelledError` to escape.

## Fix Focus Areas
- weather_briefing/llm/fallback.py[114-122]

## Suggested fix
- In the `except asyncio.CancelledError:` block, ensure `fallback.aclose()` cannot mask the original cancellation:
 - either `await asyncio.shield(self._fallback.aclose())` and catch `BaseException` (or `asyncio.CancelledError`) from the fallback close, log only safe fields, then re-raise the original cancellation; or
 - explicitly catch `asyncio.CancelledError` from `fallback.aclose()` and suppress/log it, then re-raise the primary cancellation.
- Add a regression test where `fallback.aclose.side_effect = asyncio.CancelledError()` and assert the raised exception is the original primary cancellation instance.

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

---

7. Cancellation skips fallback close ✓ Resolved 🐞 Bug ☼ Reliability

Description

FallbackLLMProvider.aclose() lets asyncio.CancelledError from the primary provider abort before
the fallback provider is closed, leaking owned resources during cancellation-driven shutdown. The
new test codifies this by asserting fallback.aclose() is not awaited when the primary close is
cancelled.

Code

weather_briefing/llm/fallback.py[R110-118]

+        errors: list[Exception] = []
+        try:
+            await self._primary.aclose()
+        except Exception as exc:
+            errors.append(exc)
+        try:
+            await self._fallback.aclose()
+        except Exception as exc:
+            errors.append(exc)

Relevance

⭐⭐⭐ High

Repo has accepted cancellation-safety fixes to prevent skipped cleanup/resource leaks; same
rationale applies to closing fallback on CancelledError.

PR-#107

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

aclose() only catches Exception, so cancellation bypasses the fallback-close path; the new test
asserts fallback is not closed on cancellation. The project’s own notes emphasize closing LLM
resources to avoid leaks, which makes skipping fallback cleanup on cancellation a practical
reliability risk.

weather_briefing/llm/fallback.py[108-122]
tests/test_llm_fallback.py[217-231]
docs/notes.md[51-57]

Agent prompt

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

## Issue description
`FallbackLLMProvider.aclose()` catches only `Exception`, so if `self._primary.aclose()` raises `asyncio.CancelledError` (a `BaseException`), the method exits immediately and never attempts to close `self._fallback`. This can leak the fallback provider's owned network/SDK resources during cancellation-driven shutdown.

## Issue Context
The wrapper is intended to own/close both providers to avoid resource leaks. The current test suite explicitly expects that cancellation prevents closing the fallback, but this behavior is risky for long-running processes and contradicts the intent to avoid leaks.

## Fix Focus Areas
- weather_briefing/llm/fallback.py[108-122]
- tests/test_llm_fallback.py[217-231]

### Implementation notes
- Handle `asyncio.CancelledError` (and possibly other `BaseException` control-flow exceptions) separately.
- Attempt best-effort closing of the fallback provider in a `finally` or via `asyncio.shield(...)`, then re-raise the cancellation so cancellation semantics are preserved.
- Update `test_close_does_not_capture_cancellation` to assert that cancellation is still raised *and* fallback cleanup is attempted (or add a new test asserting that behavior).

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

---

8. aclose captures cancellation ✓ Resolved 🐞 Bug ☼ Reliability

Description

FallbackLLMProvider.aclose() catches BaseException for each provider close; this can intercept
asyncio.CancelledError and then later raise a BaseExceptionGroup (e.g., if the other close also
fails), changing cancellation semantics and potentially delaying/shielding shutdown. Cleanup error
collection should avoid treating cancellation/control-flow exceptions as ordinary cleanup failures.

Code

weather_briefing/llm/fallback.py[R113-122]

+        except BaseException as exc:
+            errors.append(exc)
+        try:
+            await self._fallback.aclose()
+        except BaseException as exc:
+            errors.append(exc)
+        if len(errors) == 1:
+            raise errors[0]
+        if errors:
+            raise BaseExceptionGroup("Failed to close fallback LLM providers", errors)

Relevance

⭐⭐⭐ High

Team has accepted cancellation-safety fixes; avoiding CancelledError capture in cleanup matches
prior reliability expectations.

PR-#107

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new FallbackLLMProvider.aclose() uses except BaseException and may raise a
BaseExceptionGroup, which can inadvertently include cancellation/control-flow exceptions;
elsewhere, the repo’s LLM SDK cleanup helper catches only Exception, signaling the intended
pattern is to avoid capturing control-flow exceptions during cleanup.

weather_briefing/llm/fallback.py[108-122]
weather_briefing/llm/any_llm.py[223-241]

Agent prompt

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

## Issue description
`FallbackLLMProvider.aclose()` currently catches `BaseException` for both provider cleanup calls and aggregates failures. Because `BaseException` includes cancellation/control-flow exceptions (notably `asyncio.CancelledError`), cancellation can be captured into the `errors` list and later turned into a `BaseExceptionGroup` if the other provider close also fails, altering expected cancellation behavior.

## Issue Context
The repo’s other LLM cleanup helper uses `except Exception` (not `BaseException`) to avoid interfering with task failure/cancellation semantics.

## Fix Focus Areas
- weather_briefing/llm/fallback.py[108-122]

## Suggested fix approach
- Change the exception handling to:
 - Catch `Exception` for ordinary cleanup failures (to preserve current multi-error aggregation behavior for real errors).
 - Special-case `asyncio.CancelledError` (and potentially `KeyboardInterrupt`/`SystemExit`) so cancellation is re-raised (optionally after attempting to close the other provider via `asyncio.shield(...)` if you still want best-effort cleanup under cancellation).
- Ensure that when cancellation occurs, the function does not raise `BaseExceptionGroup` containing a `CancelledError` (preserve cancellation semantics).

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

---

9. FallbackLLMProvider undocumented in notes ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

A new custom cross-provider retry/fallback mechanism (FallbackLLMProvider) is introduced, but
there is no corresponding entry in docs/notes.md explaining why this custom external-service
behavior exists and what it replaces. This increases operational risk because future maintainers may
not understand the rationale or revisit triggers for the custom integration behavior.

Code

weather_briefing/llm/fallback.py[R65-66]

+        if self._using_fallback:
+            return await fallback_call()

Relevance

⭐⭐⭐ High

Repo enforces doc-compliance updates for non-trivial behavior changes; likely will add/extend notes
entry.

PR-#92
PR-#102

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The compliance rule requires non-trivial custom infrastructure for external services to be briefly
documented in docs/notes.md. The new FallbackLLMProvider implements request-failure fallback
behavior across two LLM providers, but docs/notes.md (which already documents other LLM-related
trade-offs) does not include an entry for this new fallback mechanism.

Rule 2141694: Document custom infrastructure integrations and prefer official SDKs for external services
weather_briefing/llm/fallback.py[65-70]
docs/notes.md[1-6]
docs/notes.md[41-57]

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 PR adds custom external-service integration behavior (cross-provider LLM fallback/retry via `FallbackLLMProvider`) but does not document it in `docs/notes.md` as required.

## Issue Context
`docs/notes.md` is used to record intentionally kept trade-offs and non-obvious choices; this new fallback behavior is an operationally meaningful custom mechanism around an external service.

## Fix Focus Areas
- docs/notes.md[1-6]
- docs/notes.md[41-57]
- weather_briefing/llm/fallback.py[65-66]

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

---

10. Fallback drops base/key ✓ Resolved 🐞 Bug ☼ Reliability

Description

composition.providers.llm_provider() builds the fallback any-llm adapter without
api_key/api_base, so fallback requests may ignore Settings-derived endpoint/credential overrides
and hit a different endpoint or fail to authenticate (configuration-dependent). This is especially
visible for providers like DeepSeek where the app normalizes base URL/key into Settings and
explicitly forwards them for the primary adapter but not for the fallback.

Code

weather_briefing/composition/providers.py[R60-65]

+    fallback = any_llm.create_any_llm_provider(
+        settings.llm_fallback_provider,
+        settings.llm_fallback_model,
+        settings.llm_max_output_tokens,
+        diagnostics=diagnostics,
+    )

Relevance

⭐⭐⭐ High

Likely treated as real config/credential bug; prior reviews emphasize correct llm_provider
composition parameters being forwarded.

PR-#102

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The primary any-llm adapter is created with api_key=settings.api_key and
api_base=settings.llm_base_url, but the fallback adapter is created without those parameters.
Since create_any_llm_provider() forwards api_key/api_base into AnyLLM.create(...), omitting
them changes how the SDK is configured. Additionally, Settings.from_env() only normalizes
DeepSeek’s key/base URL into Settings.api_key/Settings.llm_base_url, so dropping those values
for the fallback can make DeepSeek fallback configurations diverge from the primary configuration
behavior.

weather_briefing/composition/providers.py[45-71]
weather_briefing/config/settings.py[151-168]
weather_briefing/llm/any_llm.py[255-276]

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 fallback provider is created with fewer configuration parameters than the primary provider (no `api_key` / `api_base`). This can cause the fallback to ignore Settings-derived overrides (e.g., normalized DeepSeek base URL / key) and behave differently than the primary configuration.

## Issue Context
- Primary provider creation forwards `api_key` and `api_base` from `Settings`.
- Fallback provider creation currently forwards only `diagnostics`.
- `create_any_llm_provider()` forwards `api_key`/`api_base` directly into `AnyLLM.create(...)`, so omitting them changes runtime configuration.

## Fix Focus Areas
- weather_briefing/composition/providers.py[45-71]
- weather_briefing/config/settings.py[151-168]
- weather_briefing/llm/any_llm.py[255-276]

## Implementation notes
- Decide the intended policy:
 - If fallback should reuse the same explicit overrides when appropriate (e.g., fallback provider == primary provider, or fallback provider == "deepseek" and Settings contains DeepSeek overrides), pass `api_key`/`api_base` to the fallback `create_any_llm_provider(...)` call.
 - If fallback should support *different* credentials/endpoints, add separate settings (e.g., `LLM_FALLBACK_BASE_URL`, provider-specific fallback key/base handling) and forward those explicitly.
- Add/update tests to cover a fallback provider that requires the Settings-derived overrides (e.g., DeepSeek base URL compatibility via `DEEPSEEK_BASE_URL` vs `DEEPSEEK_API_BASE`).

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

---

11. design.md repeats requirements behavior ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The new docs/design.md paragraph restates the same fallback behavior that is already specified in
docs/requirements.md, rather than referencing it. This risks divergence over time, violating the
rule that docs/design.md should avoid duplicating functional requirements text.

Code

docs/design.md[184]

+`LLM_FALLBACK_PROVIDER` 和 `LLM_FALLBACK_MODEL` 必须同时配置。主提供商抛出 `LLMRequestError` 时，同一次操作只向备用提供商重试一次；截断、结构化输出或领域验证失败继续由原提供商进入既有修复流程。主、备用适配器各自持有并关闭 SDK 资源。

Relevance

⭐⭐⭐ High

Team previously accepted removing design.md requirement duplication; prefers referencing
requirements.md to avoid divergence.

PR-#92
PR-#102

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2141667 requires docs/design.md to remain a current technical contract and to
avoid duplicating functional requirements defined elsewhere. The added paragraph in docs/design.md
restates the fallback semantics that are also newly added as a requirement in
docs/requirements.md.

Rule 2141667: Keep docs/design.md limited to the current technical contract
docs/design.md[184-184]
docs/requirements.md[67-67]

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` repeats a functional requirement that is already documented in `docs/requirements.md` (fallback behavior). The design doc should reference the requirements doc and focus on the technical contract details that are not already stated as requirements.

## Issue Context
This duplication increases the chance that one document is updated without the other, creating an inconsistent contract.

## Fix Focus Areas
- docs/design.md[184-184]
- docs/requirements.md[67-67]

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

---

12. aclose masks cleanup error ✓ Resolved 🐞 Bug ☼ Reliability

Description

FallbackLLMProvider.aclose() uses try/finally so if both primary.aclose() and fallback.aclose()
raise, the fallback exception replaces the primary one. This can obscure the original cleanup
failure and differs from the codebase’s existing approach of avoiding cleanup errors replacing
earlier failures.

Code

weather_briefing/llm/fallback.py[R104-109]

+    async def aclose(self) -> None:
+        """Close both configured providers even when primary cleanup fails."""
+        try:
+            await self._primary.aclose()
+        finally:
+            await self._fallback.aclose()

Relevance

⭐⭐⭐ High

Team has accepted robustness fixes around try/finally cleanup; preserving primary close error is a
straightforward reliability improvement.

PR-#107

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The current try/finally will propagate the fallback close exception if it raises, even if primary
close already failed; the any-llm cleanup helper explicitly aims to not replace failures during
cleanup.

weather_briefing/llm/fallback.py[104-109]
weather_briefing/llm/any_llm.py[223-241]
tests/test_llm_fallback.py[116-146]

Agent prompt

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

### Issue description
`FallbackLLMProvider.aclose()` always runs `fallback.aclose()` in a `finally`, but if `fallback.aclose()` raises it will overwrite a failure from `primary.aclose()`. This makes cleanup failures harder to diagnose when both providers fail to close.

### Issue Context
The any-llm adapter already tries to avoid replacing failures during cleanup.

### Fix Focus Areas
- weather_briefing/llm/fallback.py[104-109]
- weather_briefing/llm/any_llm.py[223-241]
- tests/test_llm_fallback.py[116-146]

### Implementation direction
- Capture exceptions from both closes and decide a stable policy (e.g., preserve the primary exception if it occurred and attach the fallback failure as context/note, or raise an `ExceptionGroup` containing both).
- Add a test where both `primary.aclose()` and `fallback.aclose()` raise, and assert the chosen propagation/aggregation behavior.

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

---

13. Cancellation log lacks provider ✓ Resolved 🐞 Bug ◔ Observability

Description

In FallbackLLMProvider.aclose(), when fallback cleanup is cancelled after a primary close failure,
the warning logs only the primary error type and omits which primary/fallback providers were
involved, reducing attribution during shutdown debugging.

Code

weather_briefing/llm/fallback.py[R131-137]

+        except asyncio.CancelledError:
+            if errors:
+                _LOGGER.warning(
+                    "Fallback LLM provider cleanup was cancelled after primary close failure error_type=%s",
+                    type(errors[0]).__name__,
+                )
+            raise

Relevance

⭐⭐⭐ High

Team has accepted adding richer warning logs with operation/provider context for LLM failures; this
is analogous.

PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cancellation-after-primary-failure warning emitted in this path includes only error_type=%s
and does not include provider identifiers, which limits operational attribution.

weather_briefing/llm/fallback.py[129-137]

Agent prompt

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

### Issue description
`FallbackLLMProvider.aclose()` logs a warning when `fallback.aclose()` raises `CancelledError` after `primary.aclose()` already failed. The warning includes only `error_type` and omits the provider names, which makes it harder to attribute cleanup issues in multi-provider configurations.

### Issue Context
This is in the cancellation-handling path after a primary close failure.

### Fix Focus Areas
- weather_briefing/llm/fallback.py[131-136]

### Suggested fix
Update the warning message to include `primary=%s fallback=%s` and pass `self._primary_name` / `self._fallback_name` as arguments (similar to the request-fallback warning style), while continuing to avoid logging exception messages/details.

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

---

14. Fallback stickiness overstated ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The README states that after the first primary request failure “the process keeps using the fallback
until it restarts,” but the daemon/CLI creates and closes a fresh LLM provider per scheduled run, so
stickiness lasts only for the lifetime of that provider instance (typically one run). This can
mislead operators about recovery behavior (the primary will be retried on the next run without a
process restart).

Code

README.md[131]

+Fallback is disabled by default. Configure both fallback variables to retry a primary-provider request failure with the fallback. After the first such failure, the process keeps using the fallback until it restarts. Responses that violate the structured output contract stay with the provider that returned them and use the normal bounded repair attempts.

Relevance

⭐⭐⭐ High

Team often accepts fixing docs/runtime mismatches; README should not overstate process-level
behavior.

PR-#81
PR-#82
PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The README line explicitly claims process-level stickiness. In practice, llm_provider is created
inside an AsyncExitStack per run and closed when the run completes, and the daemon schedules
run() repeatedly, so fallback pinning cannot persist for the whole process unless the provider
instance itself is long-lived (which it is not in the daemon/CLI path).

README.md[131-132]
README_ja.md[131-131]
README_zh-Hans.md[131-131]
weather_briefing/cli.py[299-307]
weather_briefing/cli.py[455-474]

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 docs claim fallback remains active for the entire process until restart, but the implementation pins fallback only for the lifetime of the `FallbackLLMProvider` instance.

### Issue Context
In the daemon/CLI, each scheduled run constructs a new LLM provider and closes it at the end of the run, so later runs start with a fresh provider and will attempt the primary again.

### Fix Focus Areas
- README.md[131-131]
- README_ja.md[131-131]
- README_zh-Hans.md[131-131]

### Suggested change
Update the sentence about stickiness to say it remains on the fallback **until the LLM provider instance is recreated** (e.g., next scheduled run or process restart), rather than until process restart unconditionally. Apply the same wording adjustment in the Japanese and Simplified Chinese translations.

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

---

15. README omits sticky fallback ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

README.md describes fallback as retrying a failed primary request once, but the implementation
permanently pins the provider instance to the fallback after the first primary LLMRequestError, so
subsequent calls won’t try the primary again until the wrapper is recreated. This can mislead
operators about recovery behavior (e.g., expecting later calls to probe the primary again after a
transient outage).

Code

README.md[131]

+Fallback is disabled by default. Configure both fallback variables to retry primary-provider request failures once with the fallback. Responses that violate the structured output contract stay with the provider that returned them and use the normal bounded repair attempts.

Relevance

⭐⭐⭐ High

Team often fixes README/runtime mismatches; similar doc clarifications were accepted for env/README
behavior.

PR-#114
PR-#90

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The README’s new paragraph describes a one-time retry, while FallbackLLMProvider flips
_using_fallback to true on the first primary LLMRequestError and then routes all later requests
directly to fallback. The stickiness is also described in the design/notes docs, confirming this is
intended behavior that the README is currently not conveying.

README.md[131-131]
weather_briefing/llm/fallback.py[60-80]
docs/notes.md[51-57]
docs/design.md[184-184]

Agent prompt

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

### Issue description
`README.md` states that fallback retries primary failures once, but omits that `FallbackLLMProvider` pins all subsequent operations to the fallback provider for the lifetime of that provider instance after the first primary request failure.

### Issue Context
This behavior is implemented in `FallbackLLMProvider` and explicitly described in deeper design/notes docs, but the top-level README (and its translations) is the primary operator reference and should reflect the “sticky until recreated” recovery semantics.

### Fix Focus Areas
- README.md[131-131]
- README_ja.md[131-131]
- README_zh-Hans.md[131-131]

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

---

View more (1)

16. Non-ideal cancellation re-raise ✓ Resolved 🐞 Bug ◔ Observability

Description

In FallbackLLMProvider.aclose(), the CancelledError handler re-raises with raise cancellation,
which can add an extra re-raise frame compared to a bare raise, making cancellation tracebacks
noisier. This slightly reduces debuggability during shutdown/cancellation paths.

Code

weather_briefing/llm/fallback.py[R114-124]

+        except asyncio.CancelledError as cancellation:
+            try:
+                await self._fallback.aclose()
+            except asyncio.CancelledError:
+                _LOGGER.warning("Fallback LLM provider cleanup was cancelled while preserving primary cancellation")
+            except Exception as exc:
+                _LOGGER.warning(
+                    "Failed to close fallback LLM provider during cancellation error_type=%s",
+                    type(exc).__name__,
+                )
+            raise cancellation

Relevance

⭐⭐⭐ High

Team has accepted cancellation-safety cleanups; switching to bare raise is low-risk and improves
tracebacks.

PR-#107

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cancellation path explicitly uses raise cancellation after attempting fallback cleanup,
whereas other cancellation-safe cleanup code in the repo uses a bare raise to propagate
cancellation cleanly.

weather_briefing/llm/fallback.py[109-124]
weather_briefing/persistence/locking.py[33-41]

Agent prompt

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

### Issue description
`FallbackLLMProvider.aclose()` catches `asyncio.CancelledError` and re-raises it via `raise cancellation`. While cancellation is still propagated, this re-raise style can add an extra frame/context compared to a bare `raise`, making cancellation tracebacks slightly noisier.

### Issue Context
The codebase already uses bare `raise` for cancellation propagation in other cancellation-safe cleanup code.

### Fix Focus Areas
- weather_briefing/llm/fallback.py[114-124]

ⓘ 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 563f8f3

[IceCodeNew]

/agentic_review

[qodo-code-review]

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

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3dd2ab5

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 005dbe9

[IceCodeNew]

/agentic_review

[qodo-code-review]

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

[IceCodeNew]

/agentic_review

[qodo-code-review]

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

[qodo-code-review]

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

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 77a2a24

[qodo-code-review]

Code review by qodo was updated up to the latest commit 77a2a24

[qodo-code-review]

Code review by qodo was updated up to the latest commit 77a2a24

[IceCodeNew]

/agentic_review

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 55602fa