Skip to content

[02/10] refactor: package LLM integration - #96

Merged
IceCodeNew merged 2 commits into
masterfrom
codex/weather-refactor-04-llm
Jul 23, 2026
Merged

[02/10] refactor: package LLM integration#96
IceCodeNew merged 2 commits into
masterfrom
codex/weather-refactor-04-llm

Conversation

@IceCodeNew

@IceCodeNew IceCodeNew commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Summary

  • separate the provider contract, any-llm compatibility adapter, schema, and result conversion
  • preserve the public LLM API through the package initializer
  • remove the monolithic llm.py module

Scope

Independently based on master. Merged as step 02 in the numbered series.

Verification

  • prek run --all-files
  • 886 tests passed
  • line coverage: 99.85% (master: 99.85%)
  • branch coverage: 99.55% (master: 99.55%)

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The LLM implementation is reorganized from a single module into a package containing contracts, strict schemas, response decoding, provider integration, result mapping, and explicit public exports. A test verifies the public name of SensitiveLLMDiagnostics.

Changes

LLM package refactor

Layer / File(s) Summary
Structured LLM contract and validation
weather_briefing/llm/base.py, weather_briefing/llm/schema.py
Defines LLM errors, protocols, serialization, strict payload models, schema validation, and completion response decoding.
Validated output to briefing results
weather_briefing/llm/result.py
Validates source identifiers and timezone awareness, then maps structured output into briefing domain objects.
Any-LLM provider adapter
weather_briefing/llm/any_llm.py
Adds async completion integration, diagnostic logging gates, resource cleanup, error translation, and provider construction.
Package exports and public-name validation
weather_briefing/llm/__init__.py, tests/test_llm.py
Defines package-level exports and verifies the public SensitiveLLMDiagnostics name.

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

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately reflects the main change: refactoring the LLM integration into a package.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/weather-refactor-04-llm

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@IceCodeNew

Copy link
Copy Markdown
Owner Author

/agentic_review

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.79%. Comparing base (0e4a484) to head (6bf1a31).
⚠️ Report is 4 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #96   +/-   ##
=======================================
  Coverage   99.79%   99.79%           
=======================================
  Files          46       58   +12     
  Lines        9119     9192   +73     
  Branches      552      553    +1     
=======================================
+ Hits         9100     9173   +73     
  Misses         14       14           
  Partials        5        5           

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

@qodo-code-review

qodo-code-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 41 rules

Grey Divider


Remediation recommended

1. Missing diagnostics re-export ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
SensitiveLLMDiagnostics is defined in weather_briefing.llm.base but not exposed on the
weather_briefing.llm package, so from weather_briefing.llm import SensitiveLLMDiagnostics will
fail after this refactor. This is a public API/import-path compatibility break for any callers
relying on the previous module-level symbol being available from weather_briefing.llm.
Code

weather_briefing/llm/init.py[R3-17]

+from .any_llm import AnyLLMStructuredProvider, create_any_llm_provider
+from .base import LLMError, LLMProvider, LLMRequestError, serialize_llm_payload
+from .result import parse_result
+from .schema import LLMStructuredOutput
+
+__all__ = [
+    "AnyLLMStructuredProvider",
+    "LLMError",
+    "LLMProvider",
+    "LLMRequestError",
+    "LLMStructuredOutput",
+    "create_any_llm_provider",
+    "parse_result",
+    "serialize_llm_payload",
+]
Relevance

⭐⭐⭐ High

Refactor claims to preserve public LLM API; missing re-export is a clear import-path compatibility
break.

PR-#81
PR-#90

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The package initializer only re-exports a subset of symbols, omitting SensitiveLLMDiagnostics even
though it is still a first-class protocol in base.py and is used as a public-facing type in the
adapter constructor signature.

weather_briefing/llm/init.py[3-17]
weather_briefing/llm/base.py[25-30]
weather_briefing/llm/any_llm.py[14-48]

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

### Issue description
`SensitiveLLMDiagnostics` is defined in `weather_briefing/llm/base.py` but is not imported/re-exported from `weather_briefing/llm/__init__.py`. As a result, `from weather_briefing.llm import SensitiveLLMDiagnostics` raises `ImportError`, which is a backwards-incompatible change from the prior single-module layout.

### Issue Context
This refactor intentionally preserves the public LLM API via the package initializer, but the initializer currently omits `SensitiveLLMDiagnostics`.

### Fix Focus Areas
- weather_briefing/llm/__init__.py[3-17]
- weather_briefing/llm/base.py[25-30]

### Suggested fix
1. Import `SensitiveLLMDiagnostics` in `weather_briefing/llm/__init__.py` from `.base`.
2. Add `"SensitiveLLMDiagnostics"` to `__all__` (or at minimum bind it in the module namespace to preserve the import path).

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


Grey Divider

Qodo Logo

Comment thread weather_briefing/llm/__init__.py
@IceCodeNew
IceCodeNew force-pushed the codex/weather-refactor-03-delivery branch from b3fa6f7 to 0a6e946 Compare July 23, 2026 04:33
@IceCodeNew
IceCodeNew force-pushed the codex/weather-refactor-04-llm branch from d4a8859 to 6bf1a31 Compare July 23, 2026 04:33
@IceCodeNew
IceCodeNew changed the base branch from codex/weather-refactor-03-delivery to master July 23, 2026 04:35
@IceCodeNew
IceCodeNew marked this pull request as ready for review July 23, 2026 04:38
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Refactor LLM integration into a package while preserving the public API

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Split LLM integration into contract, schema, adapter, and result-conversion modules.
• Preserve existing weather_briefing.llm public imports via package re-exports.
• Add regression coverage for the public SensitiveLLMDiagnostics protocol.
Diagram

graph TD
  svc["App Service"] --> llm["LLM API"] --> adapter["AnyLLM Adapter"] --> sdk{{"AnyLLM SDK"}}
  llm --> result["Result Parser"] --> models["Domain Models"]
  adapter --> schema["LLM Schema"]
  result --> schema
  adapter --> apictx["API Context"]
Loading
High-Level Assessment

Packaging the LLM boundary (contracts/schema/adapter/result conversion) is the right tradeoff: it improves separation of concerns while keeping the import surface stable via weather_briefing.llm.__init__ re-exports. Alternatives like keeping a single large module or renaming the new package (and adding a compatibility shim) would either preserve the current maintainability problem or risk breaking imports; the chosen approach keeps backward compatibility and clarity.

Files changed (6) +385 / -0

Refactor (5) +380 / -0
__init__.pyRe-export stable public LLM API from new package initializer +18/-0

Re-export stable public LLM API from new package initializer

• Introduces the 'weather_briefing.llm' package entrypoint that re-exports the prior public surface (errors, protocols, schema, adapter factory, and 'parse_result'). Defines '__all__' to make the supported API explicit.

weather_briefing/llm/init.py

any_llm.pyMove any-llm adapter and provider factory into dedicated module +171/-0

Move any-llm adapter and provider factory into dedicated module

• Extracts the AnyLLM adapter implementation, including request/response logging, api call context wrapping, and best-effort client cleanup. Keeps the existing behavior for structured completion requests and error translation to 'LLMRequestError'.

weather_briefing/llm/any_llm.py

base.pyDefine application-facing LLM contracts and shared utilities +35/-0

Define application-facing LLM contracts and shared utilities

• Introduces core exceptions ('LLMError', 'LLMRequestError'), provider/diagnostics protocols, and a stable JSON serializer for request payloads. This centralizes the public contract types used across adapters and callers.

weather_briefing/llm/base.py

result.pyExtract structured-output to domain-model conversion +64/-0

Extract structured-output to domain-model conversion

• Moves 'parse_result' into its own module, validating the payload then converting it into domain objects ('BriefingResult', 'Warning', 'Advice', etc.). Preserves source-id checking and raw payload retention semantics.

weather_briefing/llm/result.py

schema.pyExtract strict structured-output schema and response decoding helpers +92/-0

Extract strict structured-output schema and response decoding helpers

• Defines the Pydantic schema for structured LLM output along with helpers to validate application payloads and decode normalized any-llm responses. Keeps strict validation behavior and maintains the same error messages via 'LLMError'/'LLMRequestError'.

weather_briefing/llm/schema.py

Tests (1) +5 / -0
test_llm.pyAdd regression test for public SensitiveLLMDiagnostics export +5/-0

Add regression test for public SensitiveLLMDiagnostics export

• Imports 'SensitiveLLMDiagnostics' from 'weather_briefing.llm' and asserts it remains publicly accessible. This guards the package refactor from unintentionally dropping a public protocol used for diagnostics wiring.

tests/test_llm.py

@qodo-code-review

Copy link
Copy Markdown

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

@IceCodeNew

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

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

Inline comments:
In `@weather_briefing/llm/any_llm.py`:
- Around line 62-95: Update AnyLLMClient.summarize around the
self._client.acompletion call to enforce an explicit request timeout, using the
adapter’s existing timeout configuration if available or a defined
boundary-level timeout. Ensure the timeout applies to the entire LLM request and
preserves the existing LLMRequestError handling for request failures.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8080128f-2b80-4a45-acac-265ee16a6b25

📥 Commits

Reviewing files that changed from the base of the PR and between 26dcad5 and 6bf1a31.

📒 Files selected for processing (7)
  • tests/test_llm.py
  • weather_briefing/llm.py
  • weather_briefing/llm/__init__.py
  • weather_briefing/llm/any_llm.py
  • weather_briefing/llm/base.py
  • weather_briefing/llm/result.py
  • weather_briefing/llm/schema.py
💤 Files with no reviewable changes (1)
  • weather_briefing/llm.py

Comment thread weather_briefing/llm/any_llm.py
@IceCodeNew
IceCodeNew merged commit 638aff9 into master Jul 23, 2026
16 checks passed
@IceCodeNew
IceCodeNew deleted the codex/weather-refactor-04-llm branch July 23, 2026 05:27
@IceCodeNew IceCodeNew changed the title [04/10] refactor: package LLM integration [02/10] refactor: package LLM integration Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant