Fix fallback for native LLM provider errors

[IceCodeNew]

Summary

• normalize completion failures from application-owned AnyLLM clients at the adapter boundary
• preserve output validation and application-side serialization errors
• retain both primary and fallback exception chains when both providers fail

Root cause

AnyLLM can propagate a provider SDK exception directly. The adapter only recognized AnyLLMError, so the configured fallback provider was bypassed when the primary returned a native SDK error such as an OpenAI-compatible HTTP 400.

Validation

• prek run --all-files
• uv run --with pytest --with pytest-cov -- pytest --cov --cov-branch --cov-report=xml
• 1203 tests passed
• line coverage: 99.90%; branch coverage: 99.66%, unchanged from master

Summary by CodeRabbit

• Bug Fixes

  • Provider request failures are now consistently reported as LLMRequestError.
  • Fallback providers activate correctly for supported completion request errors.
  • Serialization errors, output validation failures, and unrelated programming or transport errors continue to surface unchanged.
  • When fallback requests also fail, the original primary failure remains available as context.

• Documentation

  • Documented provider error handling and fallback behavior.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 13 minutes

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

How can I continue?

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

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 44db6555-121c-4280-9d84-60391b9b55b2

📥 Commits

Reviewing files that changed from the base of the PR and between e9f8a36 and 99e4751.

📒 Files selected for processing (3)

• tests/test_any_llm_provider.py
• tests/test_llm.py
• weather_briefing/llm/any_llm.py

📝 Walkthrough

Walkthrough

The AnyLLM adapter centralizes completion error normalization, preserves validation, serialization, and protocol errors, broadens completion message types, and enables request-error normalization through the provider factory. Tests cover native failures, fallback switching, and exception context.

Changes

Native Error Normalization

Layer / File(s)	Summary
Normalization contract and detection weather_briefing/llm/any_llm.py	Completion messages accept dictionaries or ChatCompletionMessage objects; the shared context manager converts configured request failures to LLMRequestError while preserving validation errors.
Provider completion integration weather_briefing/llm/any_llm.py	Structured completion methods use the shared completion path, and the provider factory enables normalization for created providers.
Error and fallback behavior validation tests/test_any_llm_provider.py, tests/test_llm.py, tests/test_llm_fallback.py, docs/notes.md	Tests cover native error conversion, preserved serialization and completion errors, fallback switching, exception context, and message typing; notes document the fallback boundary.

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

Sequence Diagram(s)

sequenceDiagram
  participant PrimaryClient
  participant AnyLLMStructuredProvider
  participant FallbackLLMProvider
  PrimaryClient->>AnyLLMStructuredProvider: completion request
  AnyLLMStructuredProvider->>AnyLLMStructuredProvider: normalize provider request error
  AnyLLMStructuredProvider->>FallbackLLMProvider: raise LLMRequestError
  FallbackLLMProvider->>FallbackLLMProvider: switch to fallback provider

Loading

Possibly related PRs

• IceCodeNew/weather-briefing#57: Handles LLMRequestError in downstream output-contract control flow.
• IceCodeNew/weather-briefing#96: Also changes AnyLLM completion error mapping.
• IceCodeNew/weather-briefing#115: Uses LLMRequestError to trigger fallback-provider switching.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 matches the main change: native provider errors are normalized so fallback can trigger.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-llm-fallback-errors

---

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.86%. Comparing base (8048bfd) to head (99e4751).
⚠️ Report is 2 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #120   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         115      115           
  Lines       12154    12239   +85     
  Branches      729      730    +1     
=======================================
+ Hits        12137    12222   +85     
  Misses         12       12           
  Partials        5        5           

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

[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. Classifier aborts on metadata ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

_is_provider_request_error wraps all probing in a single try/except, so if accessing exc.request/
exc.request_info raises, it returns False before evaluating exc.response/status metadata. This can
skip normalization (and therefore fallback) for provider-native HTTP failures that still expose
response/status but have a faulty request accessor.

Code

weather_briefing/llm/any_llm.py[R53-64]

+    try:
+        if isinstance(exc, httpx.HTTPError):
+            return True
+        request = getattr(exc, "request", None)
+        if request is None:
+            request = getattr(exc, "request_info", None)
+        if request is not None and _is_request_context(request):
+            return True
+        response = getattr(exc, "response", None)
+        return response is not None and (_has_http_status(response) or _has_http_status(exc))
+    except Exception:
+        return False

Relevance

⭐⭐⭐ High

Team recently accepted multiple fixes preventing exception-handling from masking meaningful errors;
this is aligned and low-risk.

PR-#115

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The classifier returns False on any exception during metadata probing, which can happen when
accessing exc.request raises (as demonstrated by the new _BrokenRequestMetadataError). Because
the broad try/except wraps both request inspection and response/status checks, a failure in the
request path prevents response/status classification from running.

weather_briefing/llm/any_llm.py[51-64]
tests/test_any_llm_provider.py[91-95]
tests/test_any_llm_provider.py[398-413]

Agent prompt

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

## Issue description
`_is_provider_request_error()` currently wraps *all* request/response/status probing in one `try` block. If `exc.request` (or `exc.request_info`) access raises (e.g., a property that throws), the broad `except Exception:` triggers and returns `False`, so the function never checks `exc.response` / HTTP status indicators.

This can prevent `_normalize_request_errors()` from wrapping a request failure into `LLMRequestError`, which can bypass fallback even when response/status metadata would have been sufficient to classify the exception as an HTTP/provider request error.

## Issue Context
There is already a regression test proving request-metadata inspection can raise via a property (`_BrokenRequestMetadataError.request`). The classifier should treat failures to inspect one metadata field as non-fatal and continue checking other available indicators (notably `response`/status).

## Fix Focus Areas
- weather_briefing/llm/any_llm.py[51-64]
- tests/test_any_llm_provider.py[91-96]
- tests/test_any_llm_provider.py[398-415]

## Suggested approach
- Narrow the `try/except` scope so an error reading `exc.request` does **not** prevent checking `exc.response` or `_has_http_status(exc)`.
 - Example: wrap each attribute access (`request`, `request_info`, `response`) in its own small `try/except Exception:` and treat failures as `None`.
- Add/adjust a regression test for the specific failure mode:
 - an exception whose `request` accessor raises *and* whose `response` attribute exposes an HTTP status; assert it is still normalized to `LLMRequestError` when `normalize_native_errors=True`.

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

---

2. Classifier can mask exceptions ✓ Resolved 🐞 Bug ☼ Reliability

Description

_is_request_context performs unguarded attribute access and stringification (str(url).lower()) while
classifying an already-raised exception; if the request metadata object has a raising property or a
faulty __str__, the classifier can throw a new exception and hide the original provider failure.
This breaks the intended behavior of _normalize_request_errors (and therefore fallback) by letting a
secondary exception escape instead of the original/normalized error.

Code

weather_briefing/llm/any_llm.py[R42-48]

+    method = getattr(value, "method", None)
+    if not isinstance(method, str) or method.upper() not in _HTTP_METHODS:
+        return False
+    url = getattr(value, "url", None)
+    if url is None:
+        url = getattr(value, "real_url", None)
+    return str(url).lower().startswith(("http://", "https://"))

Relevance

⭐⭐⭐ High

Team has recent accepted fixes preventing secondary exceptions from masking originals during error
handling/cleanup.

PR-#115

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The normalization path calls _is_provider_request_error() while already handling another
exception, and _is_provider_request_error() calls _is_request_context() on arbitrary
exc.request/exc.request_info objects without guarding against failures inside
_is_request_context(). As written, _is_request_context() can raise during method/url
attribute access or string conversion, replacing the original provider exception.

weather_briefing/llm/any_llm.py[40-48]
weather_briefing/llm/any_llm.py[51-61]
weather_briefing/llm/any_llm.py[80-96]

Agent prompt

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

### Issue description
`_is_request_context()` is invoked while handling an exception (via `_normalize_request_errors` → `_is_provider_request_error`). Because `_is_request_context()` does unguarded metadata inspection and `str(url).lower()`, it can raise a *new* exception (e.g., from a raising property or faulty `__str__`), masking the original provider error and preventing the intended `LLMRequestError` normalization/fallback behavior.

### Issue Context
This function runs in the exception-normalization path, so it must be defensive and “fail closed” (i.e., return `False`) rather than raising.

### Fix Focus Areas
- weather_briefing/llm/any_llm.py[40-48]

### Suggested fix
Wrap the metadata inspection and URL string conversion in a `try/except Exception` block and return `False` on classifier failures. Also consider explicitly handling `url is None` before calling `str(url)`.

Example sketch:
```py
try:
   ...
   if url is None:
       return False
   url_str = str(url)
except Exception:
   return False
return url_str.lower().startswith(("http://", "https://"))
```

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

---

3. Request heuristic too permissive ✓ Resolved 🐞 Bug ☼ Reliability

Description

_is_request_context considers any object with a string method plus a non-None url/real_url as an
HTTP request context, so unrelated SDK/programming exceptions that happen to carry similarly-shaped
metadata can be reclassified as request failures and wrapped into LLMRequestError (triggering
fallback). The PR’s new test demonstrates this by normalizing a plain Exception that just has a
SimpleNamespace request field.

Code

weather_briefing/llm/any_llm.py[R39-43]

+def _is_request_context(value: object) -> bool:
+    """Recognize request metadata shared by common HTTP client libraries."""
+    return isinstance(getattr(value, "method", None), str) and any(
+        getattr(value, name, None) is not None for name in ("url", "real_url")
+    )

Relevance

⭐⭐ Medium

Heuristic scope is subjective; no close precedent on tightening request-metadata detection for error
normalization.

PR-#115
PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The adapter’s classification treats request-shaped metadata as sufficient to mark an exception as a
provider request error. The new test constructs a generic Exception with only request.method and
request.url and asserts it is normalized, proving that non-provider exception types can be wrapped
based solely on this heuristic.

weather_briefing/llm/any_llm.py[39-56]
tests/test_any_llm_provider.py[79-83]
tests/test_any_llm_provider.py[331-346]

Agent prompt

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

### Issue description
`_is_request_context()` is used as a duck-typed signal that an exception is a provider/transport request failure. Today it only checks for `method: str` and `url/real_url != None`, which is broad enough that non-request/programming exceptions that include similarly-shaped metadata can be normalized into `LLMRequestError`, potentially masking bugs and incorrectly triggering sticky fallback.

### Issue Context
This behavior is demonstrated by the new test that normalizes `_RequestMetadataTransportError`, which is a plain `Exception` carrying a `SimpleNamespace(method=..., url=...)`.

### Fix Focus Areas
- weather_briefing/llm/any_llm.py[39-45]

### Suggested fix
Make `_is_request_context()` stricter by validating that:
- `method` is a known HTTP verb (e.g., GET/POST/PUT/PATCH/DELETE/HEAD/OPTIONS), and
- `url`/`real_url` is URL-like (e.g., `str` starting with `http://` or `https://`, or an object type commonly used for URLs such as `httpx.URL` / `yarl.URL`).

This keeps the intended normalization for real transport/provider errors while reducing false positives from arbitrary exceptions that merely carry request-shaped metadata.

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

---

View more (3)

4. Non-httpx transport bypass ✓ Resolved 🐞 Bug ☼ Reliability

Description

_normalize_request_errors only raises LLMRequestError when _is_provider_request_error recognizes the
exception, but that predicate’s request-path currently only matches exc.request when it is an
httpx.Request. Custom LLMCompletionClient implementations using other HTTP stacks (e.g.,
requests/aiohttp) can raise transport errors without httpx types or a response/status, causing the
exception to re-raise and bypass FallbackLLMProvider’s LLMRequestError-based switching.

Code

weather_briefing/llm/any_llm.py[R75-77]

+        if _is_provider_request_error(exc):
+            raise LLMRequestError(message) from exc
+        raise

Relevance

⭐⭐ Medium

Speculative non-httpx client edge case; team values robust fallback handling but no close precedent
on error-classifier broadening.

PR-#115
PR-#118

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The adapter supports protocol clients, but the request-error classifier only recognizes request
metadata when it’s an httpx.Request, so non-httpx transport failures can escape normalization and
won’t be converted into LLMRequestError (the only error type that triggers fallback switching).

weather_briefing/llm/any_llm.py[39-47]
weather_briefing/llm/any_llm.py[65-77]
weather_briefing/llm/any_llm.py[49-62]
weather_briefing/llm/fallback.py[61-85]

Agent prompt

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

### Issue description
`_normalize_request_errors()` depends on `_is_provider_request_error()` to decide whether to wrap an exception into `LLMRequestError`. Right now `_is_provider_request_error()` only treats `exc.request` as a signal when it is specifically an `httpx.Request`, which is too narrow for the `LLMCompletionClient` Protocol (which allows injected clients that may use other HTTP libraries).

Because `FallbackLLMProvider` switches providers only on `LLMRequestError`, transport failures from non-`httpx` clients can bypass fallback.

### Issue Context
The adapter explicitly supports `client: AnyLLM | LLMCompletionClient`, so non-AnyLLM implementations are part of the intended API surface.

### Fix Focus Areas
- weather_briefing/llm/any_llm.py[39-47]
- weather_briefing/llm/any_llm.py[65-77]

### Suggested change
Adjust `_is_provider_request_error()` to detect request-like objects more generically (not only `httpx.Request`). Options:
- Treat any non-None `exc.request` as a request signal when it has common request attributes (e.g., `method` and `url`/`real_url`).
- Optionally add support for common aiohttp shapes (`exc.request_info`) when present.
- Keep the existing `httpx.HTTPError` and response/status-based checks.

Add a regression test that uses a non-`httpx` exception shape (e.g., an exception class with a `request` attribute that is not `httpx.Request`) to assert it is normalized into `LLMRequestError` and would therefore trigger fallback.

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

---

5. Type-gated error normalization ✓ Resolved 🐞 Bug ☼ Reliability

Description

_normalize_request_errors only converts provider-native transport/status exceptions into
LLMRequestError when isinstance(client, AnyLLM) is true, even though the adapter accepts
arbitrary LLMCompletionClient implementations. If the completion client is a wrapper/injected
implementation (not an AnyLLM instance), provider-native HTTP/SDK failures will re-raise as their
original exception type and can bypass FallbackLLMProvider’s LLMRequestError-based switching.

Code

weather_briefing/llm/any_llm.py[R78-80]

+        if isinstance(client, AnyLLM) and _is_provider_request_error(exc):
+            raise LLMRequestError(message) from exc
+        raise

Relevance

⭐⭐ Medium

Could be intentional to avoid masking errors from injected clients; no close precedent on widening
normalization beyond AnyLLM.

PR-#115
PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code advertises support for non-AnyLLM clients via LLMCompletionClient, but the new
normalization path for provider-native request errors is conditional on `isinstance(client,
AnyLLM)`, meaning protocol-only clients will not be normalized and thus can bypass fallback
switching that depends on LLMRequestError. The test suite includes a non-AnyLLM completion stub,
demonstrating that such clients are part of the adapter’s supported surface.

weather_briefing/llm/any_llm.py[49-80]
tests/test_any_llm_provider.py[31-44]

Agent prompt

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

## Issue description
`_normalize_request_errors(...)` currently only normalizes provider-native request failures when `client` is an `AnyLLM` instance. However, the adapter surface explicitly supports non-`AnyLLM` completion clients via the `LLMCompletionClient` protocol, so wrapped/injected clients can still leak provider-native HTTP/SDK exceptions and bypass fallback (which switches on `LLMRequestError`).

## Issue Context
- The adapter allows both `AnyLLM` and protocol-based clients.
- The normalization logic is meant to ensure fallback works when provider SDK exceptions propagate.
- A concrete `isinstance(client, AnyLLM)` check is brittle against wrappers/adapters that still represent “any-llm boundary” clients.

## Fix Focus Areas
- weather_briefing/llm/any_llm.py[65-80]

### Suggested approach
- Replace the `isinstance(client, AnyLLM)` gate with an explicit capability/ownership signal (e.g., pass `normalize_native_errors: bool` into `_normalize_request_errors`, and set it from `AnyLLMStructuredProvider` based on `self._owns_client` or based on whether the client came from `create_any_llm_provider`).
- Add a regression test that uses a concrete non-`AnyLLM` `LLMCompletionClient` implementation whose `acompletion` raises an `httpx.HTTPError` (or status-bearing exception) and asserts the exception is (or is not, depending on intended policy) normalized consistently with the adapter policy.

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

---

6. Overbroad error normalization ✓ Resolved 🐞 Bug ☼ Reliability

Description

_normalize_request_errors converts any Exception raised by an AnyLLM client (besides
LengthFinishReasonError/ValidationError/AnyLLMError) into LLMRequestError, so non-request failures
inside the SDK can be treated as request failures. Because FallbackLLMProvider permanently switches
after LLMRequestError, an unexpected SDK/programming exception could incorrectly trigger sticky
fallback and make the underlying bug harder to detect.

Code

weather_briefing/llm/any_llm.py[R61-64]

+    except Exception as exc:
+        if isinstance(client, AnyLLM):
+            raise LLMRequestError(message) from exc
+        raise

Relevance

⭐⭐ Medium

Team favors careful exception boundaries, but no close precedent on narrowing AnyLLM Exception
wrapping vs fallback stickiness.

PR-#115
PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new context manager wraps arbitrary Exceptions into LLMRequestError when the client is an AnyLLM
instance; separately, FallbackLLMProvider permanently switches after any LLMRequestError, so
over-broad wrapping changes provider selection and can mask root causes.

weather_briefing/llm/any_llm.py[49-64]
weather_briefing/llm/fallback.py[61-85]

Agent prompt

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

### Issue description
`_normalize_request_errors(...)` currently wraps **any** `Exception` raised while calling an any-llm SDK client into `LLMRequestError`. This can misclassify unexpected SDK/programming failures (e.g., internal `AttributeError`/`TypeError`, misconfiguration errors) as request failures and trigger **sticky fallback**.

### Issue Context
The fallback provider switches permanently after an `LLMRequestError`, so broad exception normalization has effects beyond a single request.

### Fix Focus Areas
- weather_briefing/llm/any_llm.py[49-64]
- weather_briefing/llm/fallback.py[61-85]

### Suggested fix approach
1. Replace the broad `except Exception` normalization with a narrower classification policy for “request/transport/provider-native” exceptions.
  - Example options:
    - Normalize only `httpx.HTTPError` and known provider SDK base error types (if available in dependencies), and re-raise other exception types.
    - Or normalize exceptions that carry an HTTP response/status (e.g., have a `response` attribute of type `httpx.Response`).
2. Consider adding a privacy-safe warning log when normalization occurs for non-`AnyLLMError` exceptions, e.g., `error_type=%s` only (no message/trace), to preserve debuggability.
3. Add a regression test showing that an obvious programming exception from `client.acompletion` (e.g., `TypeError("boom")`) is **not** converted to `LLMRequestError` / does **not** trigger fallback pinning, while vendor-native request exceptions still are normalized.

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

---

Previous review results

Review updated until commit 99e4751

Results up to commit 2b4f958 ⚖️ Balanced

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

1. Overbroad error normalization ✓ Resolved 🐞 Bug ☼ Reliability

Description

_normalize_request_errors converts any Exception raised by an AnyLLM client (besides
LengthFinishReasonError/ValidationError/AnyLLMError) into LLMRequestError, so non-request failures
inside the SDK can be treated as request failures. Because FallbackLLMProvider permanently switches
after LLMRequestError, an unexpected SDK/programming exception could incorrectly trigger sticky
fallback and make the underlying bug harder to detect.

Code

weather_briefing/llm/any_llm.py[R61-64]

+    except Exception as exc:
+        if isinstance(client, AnyLLM):
+            raise LLMRequestError(message) from exc
+        raise

Relevance

⭐⭐ Medium

Team favors careful exception boundaries, but no close precedent on narrowing AnyLLM Exception
wrapping vs fallback stickiness.

PR-#115
PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new context manager wraps arbitrary Exceptions into LLMRequestError when the client is an AnyLLM
instance; separately, FallbackLLMProvider permanently switches after any LLMRequestError, so
over-broad wrapping changes provider selection and can mask root causes.

weather_briefing/llm/any_llm.py[49-64]
weather_briefing/llm/fallback.py[61-85]

Agent prompt

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

### Issue description
`_normalize_request_errors(...)` currently wraps **any** `Exception` raised while calling an any-llm SDK client into `LLMRequestError`. This can misclassify unexpected SDK/programming failures (e.g., internal `AttributeError`/`TypeError`, misconfiguration errors) as request failures and trigger **sticky fallback**.

### Issue Context
The fallback provider switches permanently after an `LLMRequestError`, so broad exception normalization has effects beyond a single request.

### Fix Focus Areas
- weather_briefing/llm/any_llm.py[49-64]
- weather_briefing/llm/fallback.py[61-85]

### Suggested fix approach
1. Replace the broad `except Exception` normalization with a narrower classification policy for “request/transport/provider-native” exceptions.
  - Example options:
    - Normalize only `httpx.HTTPError` and known provider SDK base error types (if available in dependencies), and re-raise other exception types.
    - Or normalize exceptions that carry an HTTP response/status (e.g., have a `response` attribute of type `httpx.Response`).
2. Consider adding a privacy-safe warning log when normalization occurs for non-`AnyLLMError` exceptions, e.g., `error_type=%s` only (no message/trace), to preserve debuggability.
3. Add a regression test showing that an obvious programming exception from `client.acompletion` (e.g., `TypeError("boom")`) is **not** converted to `LLMRequestError` / does **not** trigger fallback pinning, while vendor-native request exceptions still are normalized.

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

---

Results up to commit 26d5701 ⚖️ Balanced

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

1. Type-gated error normalization ✓ Resolved 🐞 Bug ☼ Reliability

Description

_normalize_request_errors only converts provider-native transport/status exceptions into
LLMRequestError when isinstance(client, AnyLLM) is true, even though the adapter accepts
arbitrary LLMCompletionClient implementations. If the completion client is a wrapper/injected
implementation (not an AnyLLM instance), provider-native HTTP/SDK failures will re-raise as their
original exception type and can bypass FallbackLLMProvider’s LLMRequestError-based switching.

Code

weather_briefing/llm/any_llm.py[R78-80]

+        if isinstance(client, AnyLLM) and _is_provider_request_error(exc):
+            raise LLMRequestError(message) from exc
+        raise

Relevance

⭐⭐ Medium

Could be intentional to avoid masking errors from injected clients; no close precedent on widening
normalization beyond AnyLLM.

PR-#115
PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code advertises support for non-AnyLLM clients via LLMCompletionClient, but the new
normalization path for provider-native request errors is conditional on `isinstance(client,
AnyLLM)`, meaning protocol-only clients will not be normalized and thus can bypass fallback
switching that depends on LLMRequestError. The test suite includes a non-AnyLLM completion stub,
demonstrating that such clients are part of the adapter’s supported surface.

weather_briefing/llm/any_llm.py[49-80]
tests/test_any_llm_provider.py[31-44]

Agent prompt

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

## Issue description
`_normalize_request_errors(...)` currently only normalizes provider-native request failures when `client` is an `AnyLLM` instance. However, the adapter surface explicitly supports non-`AnyLLM` completion clients via the `LLMCompletionClient` protocol, so wrapped/injected clients can still leak provider-native HTTP/SDK exceptions and bypass fallback (which switches on `LLMRequestError`).

## Issue Context
- The adapter allows both `AnyLLM` and protocol-based clients.
- The normalization logic is meant to ensure fallback works when provider SDK exceptions propagate.
- A concrete `isinstance(client, AnyLLM)` check is brittle against wrappers/adapters that still represent “any-llm boundary” clients.

## Fix Focus Areas
- weather_briefing/llm/any_llm.py[65-80]

### Suggested approach
- Replace the `isinstance(client, AnyLLM)` gate with an explicit capability/ownership signal (e.g., pass `normalize_native_errors: bool` into `_normalize_request_errors`, and set it from `AnyLLMStructuredProvider` based on `self._owns_client` or based on whether the client came from `create_any_llm_provider`).
- Add a regression test that uses a concrete non-`AnyLLM` `LLMCompletionClient` implementation whose `acompletion` raises an `httpx.HTTPError` (or status-bearing exception) and asserts the exception is (or is not, depending on intended policy) normalized consistently with the adapter policy.

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

---

Results up to commit 1ebc5f5 ⚖️ Balanced

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

1. Non-httpx transport bypass ✓ Resolved 🐞 Bug ☼ Reliability

Description

_normalize_request_errors only raises LLMRequestError when _is_provider_request_error recognizes the
exception, but that predicate’s request-path currently only matches exc.request when it is an
httpx.Request. Custom LLMCompletionClient implementations using other HTTP stacks (e.g.,
requests/aiohttp) can raise transport errors without httpx types or a response/status, causing the
exception to re-raise and bypass FallbackLLMProvider’s LLMRequestError-based switching.

Code

weather_briefing/llm/any_llm.py[R75-77]

+        if _is_provider_request_error(exc):
+            raise LLMRequestError(message) from exc
+        raise

Relevance

⭐⭐ Medium

Speculative non-httpx client edge case; team values robust fallback handling but no close precedent
on error-classifier broadening.

PR-#115
PR-#118

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The adapter supports protocol clients, but the request-error classifier only recognizes request
metadata when it’s an httpx.Request, so non-httpx transport failures can escape normalization and
won’t be converted into LLMRequestError (the only error type that triggers fallback switching).

weather_briefing/llm/any_llm.py[39-47]
weather_briefing/llm/any_llm.py[65-77]
weather_briefing/llm/any_llm.py[49-62]
weather_briefing/llm/fallback.py[61-85]

Agent prompt

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

### Issue description
`_normalize_request_errors()` depends on `_is_provider_request_error()` to decide whether to wrap an exception into `LLMRequestError`. Right now `_is_provider_request_error()` only treats `exc.request` as a signal when it is specifically an `httpx.Request`, which is too narrow for the `LLMCompletionClient` Protocol (which allows injected clients that may use other HTTP libraries).

Because `FallbackLLMProvider` switches providers only on `LLMRequestError`, transport failures from non-`httpx` clients can bypass fallback.

### Issue Context
The adapter explicitly supports `client: AnyLLM | LLMCompletionClient`, so non-AnyLLM implementations are part of the intended API surface.

### Fix Focus Areas
- weather_briefing/llm/any_llm.py[39-47]
- weather_briefing/llm/any_llm.py[65-77]

### Suggested change
Adjust `_is_provider_request_error()` to detect request-like objects more generically (not only `httpx.Request`). Options:
- Treat any non-None `exc.request` as a request signal when it has common request attributes (e.g., `method` and `url`/`real_url`).
- Optionally add support for common aiohttp shapes (`exc.request_info`) when present.
- Keep the existing `httpx.HTTPError` and response/status-based checks.

Add a regression test that uses a non-`httpx` exception shape (e.g., an exception class with a `request` attribute that is not `httpx.Request`) to assert it is normalized into `LLMRequestError` and would therefore trigger fallback.

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

---

Results up to commit 681514f ⚖️ Balanced

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

1. Request heuristic too permissive ✓ Resolved 🐞 Bug ☼ Reliability

Description

_is_request_context considers any object with a string method plus a non-None url/real_url as an
HTTP request context, so unrelated SDK/programming exceptions that happen to carry similarly-shaped
metadata can be reclassified as request failures and wrapped into LLMRequestError (triggering
fallback). The PR’s new test demonstrates this by normalizing a plain Exception that just has a
SimpleNamespace request field.

Code

weather_briefing/llm/any_llm.py[R39-43]

+def _is_request_context(value: object) -> bool:
+    """Recognize request metadata shared by common HTTP client libraries."""
+    return isinstance(getattr(value, "method", None), str) and any(
+        getattr(value, name, None) is not None for name in ("url", "real_url")
+    )

Relevance

⭐⭐ Medium

Heuristic scope is subjective; no close precedent on tightening request-metadata detection for error
normalization.

PR-#115
PR-#114

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The adapter’s classification treats request-shaped metadata as sufficient to mark an exception as a
provider request error. The new test constructs a generic Exception with only request.method and
request.url and asserts it is normalized, proving that non-provider exception types can be wrapped
based solely on this heuristic.

weather_briefing/llm/any_llm.py[39-56]
tests/test_any_llm_provider.py[79-83]
tests/test_any_llm_provider.py[331-346]

Agent prompt

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

### Issue description
`_is_request_context()` is used as a duck-typed signal that an exception is a provider/transport request failure. Today it only checks for `method: str` and `url/real_url != None`, which is broad enough that non-request/programming exceptions that include similarly-shaped metadata can be normalized into `LLMRequestError`, potentially masking bugs and incorrectly triggering sticky fallback.

### Issue Context
This behavior is demonstrated by the new test that normalizes `_RequestMetadataTransportError`, which is a plain `Exception` carrying a `SimpleNamespace(method=..., url=...)`.

### Fix Focus Areas
- weather_briefing/llm/any_llm.py[39-45]

### Suggested fix
Make `_is_request_context()` stricter by validating that:
- `method` is a known HTTP verb (e.g., GET/POST/PUT/PATCH/DELETE/HEAD/OPTIONS), and
- `url`/`real_url` is URL-like (e.g., `str` starting with `http://` or `https://`, or an object type commonly used for URLs such as `httpx.URL` / `yarl.URL`).

This keeps the intended normalization for real transport/provider errors while reducing false positives from arbitrary exceptions that merely carry request-shaped metadata.

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

---

Results up to commit d7ebed6 ⚖️ Balanced

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

1. Classifier can mask exceptions ✓ Resolved 🐞 Bug ☼ Reliability

Description

_is_request_context performs unguarded attribute access and stringification (str(url).lower()) while
classifying an already-raised exception; if the request metadata object has a raising property or a
faulty __str__, the classifier can throw a new exception and hide the original provider failure.
This breaks the intended behavior of _normalize_request_errors (and therefore fallback) by letting a
secondary exception escape instead of the original/normalized error.

Code

weather_briefing/llm/any_llm.py[R42-48]

+    method = getattr(value, "method", None)
+    if not isinstance(method, str) or method.upper() not in _HTTP_METHODS:
+        return False
+    url = getattr(value, "url", None)
+    if url is None:
+        url = getattr(value, "real_url", None)
+    return str(url).lower().startswith(("http://", "https://"))

Relevance

⭐⭐⭐ High

Team has recent accepted fixes preventing secondary exceptions from masking originals during error
handling/cleanup.

PR-#115

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The normalization path calls _is_provider_request_error() while already handling another
exception, and _is_provider_request_error() calls _is_request_context() on arbitrary
exc.request/exc.request_info objects without guarding against failures inside
_is_request_context(). As written, _is_request_context() can raise during method/url
attribute access or string conversion, replacing the original provider exception.

weather_briefing/llm/any_llm.py[40-48]
weather_briefing/llm/any_llm.py[51-61]
weather_briefing/llm/any_llm.py[80-96]

Agent prompt

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

### Issue description
`_is_request_context()` is invoked while handling an exception (via `_normalize_request_errors` → `_is_provider_request_error`). Because `_is_request_context()` does unguarded metadata inspection and `str(url).lower()`, it can raise a *new* exception (e.g., from a raising property or faulty `__str__`), masking the original provider error and preventing the intended `LLMRequestError` normalization/fallback behavior.

### Issue Context
This function runs in the exception-normalization path, so it must be defensive and “fail closed” (i.e., return `False`) rather than raising.

### Fix Focus Areas
- weather_briefing/llm/any_llm.py[40-48]

### Suggested fix
Wrap the metadata inspection and URL string conversion in a `try/except Exception` block and return `False` on classifier failures. Also consider explicitly handling `url is None` before calling `str(url)`.

Example sketch:
```py
try:
   ...
   if url is None:
       return False
   url_str = str(url)
except Exception:
   return False
return url_str.lower().startswith(("http://", "https://"))
```

ⓘ 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 26d5701

[IceCodeNew]

/agentic_review

[qodo-code-review]

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

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 681514f

[IceCodeNew]

/agentic_review

[qodo-code-review]

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

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4da2b82

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 4da2b82

[IceCodeNew]

/agentic_review

[qodo-code-review]

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

[qodo-code-review]

PR Summary by Qodo

Fix AnyLLM native provider errors to preserve fallback routing

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Normalize provider SDK request failures into LLMRequestError at the AnyLLM adapter boundary.
• Preserve serialization, protocol-injected errors, and Pydantic output validation failures
 unchanged.
• Keep primary+fallback exception chains when both providers fail; add regression tests and notes.

Diagram

graph TD
  A([Application code]) --> B["FallbackLLMProvider"] --> C["AnyLLMStructuredProvider (primary)"] --> D{{"Request error normalization"}} --> E["AnyLLM / Provider SDK"]
  B --> F["AnyLLMStructuredProvider (fallback)"] --> D
  subgraph Legend
    direction LR
    _app([Caller]) ~~~ _svc[Component] ~~~ _gate{{Error boundary}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Rely on AnyLLM unified exception mode only

• ➕ Less custom normalization logic in the adapter
• ➕ Centralized error classification in the upstream library
• ➖ Still fragile if AnyLLM leaks native SDK exceptions in some paths/configs
• ➖ Harder to guarantee application-level invariants for fallback triggering

2. Move normalization into FallbackLLMProvider

• ➕ Keeps adapter behavior closer to upstream AnyLLM semantics
• ➖ Fallback would need provider-specific heuristics to detect request failures
• ➖ Harder to preserve “don’t fallback on serialization/validation” boundaries

3. Adopt a routing/orchestration layer across providers

• ➕ Supports richer failover (cooldowns, health checks, probing primary recovery)
• ➖ More complex architecture than needed for current sticky-fallback model
• ➖ Would duplicate/overlap with any-llm evolution if it adds routing later

Recommendation: Keep normalization at the AnyLLMStructuredProvider boundary (current approach). It’s the narrowest place that can reliably distinguish request-time provider failures from application serialization and output validation errors, while preserving the fallback provider’s simple contract (trigger only on LLMRequestError) and retaining both exception chains when failover also fails.

Files changed (5) +300 / -63

Bug fix (1) +95 / -54

any_llm.pyNormalize provider SDK request errors without masking validation/serialization failures +95/-54

Normalize provider SDK request errors without masking validation/serialization failures

• Adds a shared completion helper and a context-managed error normalization boundary that maps AnyLLMError (and optionally other completion-time exceptions) to LLMRequestError. Ensures LengthFinishReasonError and Pydantic ValidationError propagate, and enables normalization explicitly for factory-created (application-owned) AnyLLM clients so fallback routing is not bypassed by native SDK exceptions.

weather_briefing/llm/any_llm.py

Tests (3) +203 / -9

test_any_llm_provider.pyAdd regression tests for native provider errors and boundary behavior +190/-3

Add regression tests for native provider errors and boundary behavior

• Introduces parametrized coverage for OpenAI/Anthropic/httpx/custom provider errors to ensure they become LLMRequestError at the factory-created adapter boundary. Adds tests ensuring output validation errors and payload serialization errors are not masked, and verifies fallback switching works when the primary raises a native SDK BadRequestError.

tests/test_any_llm_provider.py

test_llm.pyTighten test payload typing +1/-2

Tighten test payload typing

• Replaces a broad Any-typed payload helper with dict[str, object] to keep tests aligned with the structured LLM payload contract.

tests/test_llm.py

test_llm_fallback.pyAssert primary+fallback exception chains are preserved +12/-4

Assert primary+fallback exception chains are preserved

• Updates the fallback failure test to include explicit causes on both primary and fallback LLMRequestError instances. Verifies the fallback failure keeps its own cause while retaining the primary failure as __context__ (including the primary cause).

tests/test_llm_fallback.py

Documentation (1) +2 / -0

notes.mdDocument normalized provider failures and fallback boundaries +2/-0

Document normalized provider failures and fallback boundaries

• Adds an explicit note that application-owned AnyLLM client exceptions during completion are normalized to LLMRequestError so fallback can activate. Clarifies that payload serialization errors, Pydantic structured-output validation errors, and injected protocol client failures propagate unchanged.

docs/notes.md

[qodo-code-review]

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

[coderabbitai]

🧹 Nitpick comments (1)

weather_briefing/llm/any_llm.py (1)

155-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract a shared completion-call helper to remove triplicated boilerplate. summarize, assess_notification, and translate_service_status all repeat the same "build messages → wrap acompletion in _normalize_request_errors+api_call_context → catch LengthFinishReasonError" shape; consolidating this avoids the three call sites drifting apart when normalization or logging logic changes.

• weather_briefing/llm/any_llm.py#L155-L169: factor the _normalize_request_errors/api_call_context/acompletion wrapping used here into a shared private helper (e.g. _complete(...)) parameterized by message text, messages, response_format, temperature, and max_tokens.
• weather_briefing/llm/any_llm.py#L195-L212: route assess_notification's completion call through the same shared helper instead of repeating the wrapping.
• weather_briefing/llm/any_llm.py#L243-L267: route translate_service_status's completion call through the same shared helper instead of repeating the wrapping.

♻️ Sketch of a shared helper

async def _complete(
    self,
    message: str,
    messages: list[dict[str, Any] | ChatCompletionMessage],
    *,
    response_format: type[BaseModel],
    temperature: float,
    max_tokens: int,
    limit_message: str,
) -> object:
    try:
        with (
            _normalize_request_errors(message, normalize_native_errors=self._normalize_native_errors),
            api_call_context(self._provider, "chat-completions"),
        ):
            return await self._client.acompletion(
                model=self._model,
                messages=messages,
                response_format=response_format,
                temperature=temperature,
                max_tokens=max_tokens,
            )
    except LengthFinishReasonError as exc:
        _LOGGER.warning("%s: provider=%s model=%r error_type=%s", limit_message, self._provider, self._model, type(exc).__name__)
        raise LLMOutputLimitError(limit_message) from exc

🤖 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/llm/any_llm.py` around lines 155 - 169, In
weather_briefing/llm/any_llm.py at lines 155-169, extract the shared completion
flow into a private _complete helper accepting the request message, messages,
response format, temperature, max_tokens, and limit message, while preserving
error normalization, api_call_context, LengthFinishReasonError logging, and
LLMOutputLimitError conversion; update assess_notification at lines 195-212 and
translate_service_status at lines 243-267 to call this helper instead of
duplicating the wrapper and exception handling.

🤖 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/llm/any_llm.py`:
- Around line 155-169: In weather_briefing/llm/any_llm.py at lines 155-169,
extract the shared completion flow into a private _complete helper accepting the
request message, messages, response format, temperature, max_tokens, and limit
message, while preserving error normalization, api_call_context,
LengthFinishReasonError logging, and LLMOutputLimitError conversion; update
assess_notification at lines 195-212 and translate_service_status at lines
243-267 to call this helper instead of duplicating the wrapper and exception
handling.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e9c38705-d6b3-40be-9c73-9f2f235772bf

📥 Commits

Reviewing files that changed from the base of the PR and between 02ecbc0 and a051749.

📒 Files selected for processing (5)

• docs/notes.md
• tests/test_any_llm_provider.py
• tests/test_llm.py
• tests/test_llm_fallback.py
• weather_briefing/llm/any_llm.py

[qodo-code-review]

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

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 99e4751

[IceCodeNew]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

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