refactor(llm): decompose litellm_client.py into mixins + leaves behind a facade (Tier 2.5) - #273
Merged
Merged
Conversation
…Tier-2.5 Task 0) Add test_litellm_client_surface.py: a single contract test asserting the full importer surface of the litellm_client facade stays importable through the decomposition — the 5 public names (LiteLLMClient, LiteLLMConfig, LiteLLMClientError, ToolCallingChatResponse, create_litellm_client) plus the 8 test-imported internals (AST-scanned from both repos). Also asserts __all__ == public set, hasattr(litellm_client, 'litellm') (SINK-3), the server/llm package re-export, per-new-module cold-import, and boot smoke on both import paths. Includes SINK-2 identity asserts (snapshot classes + _TRUNCATION_WARNED_MODELS) that skip until the leaf/mixin modules exist and enforce object identity once they do. Adds __all__ to the facade to pin the public surface.
…2.5 Task 1) Move to three dependency-free sibling leaf modules, re-exported by import binding (SINK-2, identity-preserving) so the facade surface is unchanged: - _litellm_types.py: LiteLLMConfig, ToolCallingChatResponse, LiteLLMClientError, StructuredOutputParseError, LLMHardTimeoutError. - _litellm_subprocess.py: the 6 _Completion*Snapshot dataclasses + snapshot builders + _litellm_completion_worker (calls litellm.completion via the module attr so the fork-inherited global LLM mock still intercepts). - _litellm_json_extraction.py: the 6 pure JSON helpers (were self-free methods; now module-level free functions) + _PYTHON_TO_JSON_REPLACEMENTS. The facade re-imports every moved name (the 6 snapshots by redundant-alias re-export) and _maybe_parse_structured_output now calls the JSON free functions. Surface guard extended: snapshot-class identity asserts now enforce each facade _Completion*Snapshot IS the _litellm_subprocess class. Zero behavior change, zero SINK-1 (no patch-namespace move). test_litellm_client_unit updated to call the two directly-tested JSON helpers as free functions (imported from the facade, same objects).
…5 Task 2) Move the three self-bound embedding methods (_resolve_default_embedding_model, get_embedding, get_embeddings) into EmbeddingMixin and the four stateless token-budget helpers (+ _TRUNCATION_WARNED_MODELS + budget constants) to _litellm_embedding.py; compose EmbeddingMixin into LiteLLMClient. The mixin self-types foreign members (config, _default_embedding_model, _resolve_api_key) via per-mixin TYPE_CHECKING stubs (Tier-1b idiom), never class-level defaults. SINK-1 (patch-where-used): repoint every embedding patch namespace from the old litellm_client namespace to _litellm_embedding — resolve_model_name (the :717 embedding-path site), get_service_embeddings, should_use_embedding_service, _is_chromadb_importable, NomicEmbedder, LocalEmbedder(.get), and litellm.get_model_info — across test_litellm_client_unit, test_embedding_service_provider, and test_local_embedding_provider (both the string-target and setattr(module,...) forms). The suites' hard mock assertions (assert_not_called / assert_called_once / cache-count == 1) confirm each repointed mock fires — a no-op patch hitting the real embedder/137M model would fail loud. SINK-2 (identity): the facade re-exports _TRUNCATION_WARNED_MODELS + the three test-imported budget functions by import binding; litellm.embedding stays a shared-module attr so the global mock still intercepts. Surface guard's _TRUNCATION_WARNED_MODELS identity assert now enforces object identity.
Move the response-format selection helpers (_supports_response_schema, _provider_for_model, _accepts_json_schema_response_format, _provider_response_format), the post-hoc parse orchestrator (_maybe_parse_structured_output), and the _JSON_SCHEMA_PROVIDER_ALLOWLIST class constant into StructuredOutputMixin in _litellm_structured_output.py; compose it into LiteLLMClient. The mixin self-types config via a per-mixin TYPE_CHECKING stub. Moves BEFORE text-gen since _provider_response_format and _maybe_parse_structured_output are the cross-mixin edges text-gen depends on. SINK-1 (patch-where-used): repoint assert_provider_safe_schema (:1184/:1209) from the old litellm_client namespace to _litellm_structured_output — the real guard raises under pytest, so a no-op patch would fail loud. litellm.supports_response_ schema/get_llm_provider stay shared-module attrs. Bodies verbatim. Facade drops now-unused imports (assert_provider_safe_schema, strict_response_format_for_model, json, lru_cache) and re-exports the two test-imported JSON helpers by alias. _make_request / _build_completion_params reach the moved methods via mixin MRO (self dispatch), unchanged.
…-2.5 Task 4) Move the ~17 text-generation methods (generate_response, generate_chat_response, _build_completion_params, the hard-timeout/subprocess-isolation cluster, observability, _make_request, prompt caching, multimodal image handling, _is_temperature_restricted_model) plus _compute_cost_usd (retained method, verbatim billing exception->None semantics — no separate cost module), _MODEL_TIMEOUT_FLOOR_SECONDS, and the SUPPORTED_IMAGE_FORMATS / TEMPERATURE_RESTRICTED_MODELS class constants into TextGenerationMixin (_litellm_text_generation.py). Compose it first in the MRO: LiteLLMClient(TextGenerationMixin, EmbeddingMixin, StructuredOutputMixin). Per-mixin TYPE_CHECKING stubs (Tier-1b idiom) self-type the foreign members: client-core config/logger/_api_key/_api_base/_api_version/_resolve_api_key (on the facade) and the two cross-mixin edges resolved via MRO (_provider_response_format + _maybe_parse_structured_output on StructuredOutputMixin). LLM-mock: every litellm call stays a shared-module attr (litellm.completion); the subprocess worker is imported from _litellm_subprocess. SINK-1: resolve_model_name is imported into this module for the model-role path (no test patches the text-gen binding; the :717 embedding site was repointed in Task 2). The facade is now the client-core shell: __init__ (keeps import litellm + suppress_debug_info + the Braintrust callbacks init — SINK-3), _resolve_api_key, _resolve_by_prefix (+ _SIMPLE_PROVIDER_PREFIXES), update_config, get_model, get_config, create_litellm_client, __all__, and the identity re-exports of every moved public + test-internal name. Bodies verbatim; all LLM suites + billing/cost + mock-compliance snapshots green.
📝 WalkthroughWalkthroughThe monolithic litellm_client.py module is split into separate _litellm_embedding, _litellm_json_extraction, _litellm_structured_output, _litellm_subprocess, _litellm_text_generation, and _litellm_types modules composed as mixins. litellm_client.py becomes a re-export facade preserving identity. Tests are updated to patch the new module paths, and a new facade-surface test is added. ChangesLiteLLM client module split
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TextGenerationMixin
participant CompletionSubprocess
participant litellm
participant StructuredOutputMixin
Caller->>TextGenerationMixin: generate_chat_response(messages)
TextGenerationMixin->>TextGenerationMixin: _build_completion_params()
TextGenerationMixin->>TextGenerationMixin: _make_request(params)
TextGenerationMixin->>CompletionSubprocess: _completion_with_hard_timeout(params)
CompletionSubprocess->>litellm: completion(**params) in child process
litellm-->>CompletionSubprocess: response or error
CompletionSubprocess-->>TextGenerationMixin: result or LLMHardTimeoutError
TextGenerationMixin->>StructuredOutputMixin: _maybe_parse_structured_output(content)
StructuredOutputMixin-->>TextGenerationMixin: parsed BaseModel or raw content
TextGenerationMixin-->>Caller: ToolCallingChatResponse
sequenceDiagram
participant Caller
participant EmbeddingMixin
participant EmbeddingService
participant LocalEmbedder
participant litellm
Caller->>EmbeddingMixin: get_embedding(text, model)
EmbeddingMixin->>EmbeddingMixin: embedding_provider_mode()/should_use_embedding_service()
alt service mode
EmbeddingMixin->>EmbeddingService: get_service_embeddings(text)
EmbeddingService-->>EmbeddingMixin: embedding vector
else local model
EmbeddingMixin->>LocalEmbedder: get(text)
LocalEmbedder-->>EmbeddingMixin: embedding vector
else litellm model
EmbeddingMixin->>EmbeddingMixin: _truncate_for_embedding(text)
EmbeddingMixin->>litellm: embedding(model, text)
litellm-->>EmbeddingMixin: response.data
end
EmbeddingMixin-->>Caller: embedding vector
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
reflexio/server/llm/_litellm_text_generation.py (1)
87-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake these constants immutable class variables.
Ruff is flagging these as mutable class attributes. Since they are process-wide policy constants, prefer
ClassVar[frozenset[str]].♻️ Proposed refactor
-from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, ClassVar @@ - SUPPORTED_IMAGE_FORMATS: set[str] = set(SUPPORTED_IMAGE_MIME_TYPES.keys()) + SUPPORTED_IMAGE_FORMATS: ClassVar[frozenset[str]] = frozenset( + SUPPORTED_IMAGE_MIME_TYPES + ) @@ - TEMPERATURE_RESTRICTED_MODELS = { + TEMPERATURE_RESTRICTED_MODELS: ClassVar[frozenset[str]] = frozenset({ "gpt-5", "gpt-5.4-mini", "gpt-5-nano", "gpt-5-codex", "gemini-3-flash-preview", "gemini-3-pro-preview", - } + })🤖 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 `@reflexio/server/llm/_litellm_text_generation.py` around lines 87 - 97, The constants in the text generation module are being treated as mutable class attributes, so update the definitions for SUPPORTED_IMAGE_FORMATS and TEMPERATURE_RESTRICTED_MODELS to be immutable class variables using ClassVar[frozenset[str]]. Keep their values process-wide policy constants, and adjust the type annotations in the class where these symbols are defined so Ruff no longer flags them.Source: Linters/SAST tools
reflexio/server/llm/litellm_client.py (1)
105-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSort
__all__to satisfy RUF022.Ruff flags this list as unsorted. Trivial fix, keeps the "clean ruff" claim in the PR description accurate.
🔧 Proposed fix
__all__ = [ "LiteLLMClient", + "LiteLLMClientError", "LiteLLMConfig", - "LiteLLMClientError", "ToolCallingChatResponse", "create_litellm_client", ]🤖 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 `@reflexio/server/llm/litellm_client.py` around lines 105 - 111, The __all__ export list in LiteLLMClient module is unsorted and triggers RUF022. Reorder the names in the LiteLLMClient/__all__ declaration alphabetically while keeping the same exports and structure, so Ruff passes without changing behavior.Source: Linters/SAST tools
🤖 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 `@reflexio/server/llm/_litellm_embedding.py`:
- Line 96: The remaining Ruff findings in the LiteLLM embedding module come from
a broad fallback catch and several exception messages that are not compliant. In
the fallback around the LiteLLM lookup, either narrow the bare Exception
handling to the specific error type(s) or keep it and add a clear suppression
rationale for BLE001 if the catch is intentional. Also update the
exception-raising paths in the module so the messages satisfy RUF010 by using
proper exception message formatting consistently in the affected lookup and
validation branches.
In `@reflexio/server/llm/_litellm_text_generation.py`:
- Around line 229-236: Avoid mutating caller-owned message dictionaries in the
message assembly logic. In _litellm_text_generation.py, final_messages =
list(messages) only copies the list, so updating final_messages[0]["content"]
still changes the original first message when it is already a system message.
Update the code in the system-message merge path to copy that first message dict
before modifying it, so reused histories in final_messages and the caller’s
messages stay untouched.
- Around line 865-870: Reject plaintext image URLs in the image handling branch
of _litellm_text_generation before adding them to content_blocks. Update the
image.startswith(...) check so only secure https:// URLs are forwarded as
{"type": "image_url"} entries, and treat http:// as invalid or unsupported in
this path. Keep the change localized to the URL handling logic near
content_blocks.append and preserve the existing file-path handling for non-URL
images.
- Around line 471-484: The hard-timeout handling in the LLM worker flow can
falsely time out because the child process is joined before draining
result_queue. Update the logic around the process.join and result_queue.get
sequence in the text generation path so the queue is read/drained before waiting
on the child process, and keep the timeout/kill cleanup in the same flow. Use
the existing hard timeout block and the result_queue handling in
_litellm_text_generation to locate the fix.
---
Nitpick comments:
In `@reflexio/server/llm/_litellm_text_generation.py`:
- Around line 87-97: The constants in the text generation module are being
treated as mutable class attributes, so update the definitions for
SUPPORTED_IMAGE_FORMATS and TEMPERATURE_RESTRICTED_MODELS to be immutable class
variables using ClassVar[frozenset[str]]. Keep their values process-wide policy
constants, and adjust the type annotations in the class where these symbols are
defined so Ruff no longer flags them.
In `@reflexio/server/llm/litellm_client.py`:
- Around line 105-111: The __all__ export list in LiteLLMClient module is
unsorted and triggers RUF022. Reorder the names in the LiteLLMClient/__all__
declaration alphabetically while keeping the same exports and structure, so Ruff
passes without changing behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e7ae84c-487a-4521-89d4-79995b75407b
📒 Files selected for processing (11)
reflexio/server/llm/_litellm_embedding.pyreflexio/server/llm/_litellm_json_extraction.pyreflexio/server/llm/_litellm_structured_output.pyreflexio/server/llm/_litellm_subprocess.pyreflexio/server/llm/_litellm_text_generation.pyreflexio/server/llm/_litellm_types.pyreflexio/server/llm/litellm_client.pytests/server/llm/test_embedding_service_provider.pytests/server/llm/test_litellm_client_surface.pytests/server/llm/test_litellm_client_unit.pytests/server/llm/test_local_embedding_provider.py
| """ | ||
| try: | ||
| info = litellm.get_model_info(model) | ||
| except Exception: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the remaining Ruff findings in this module.
Line 96 still trips BLE001, and the exception messages here trip RUF010. If the broad LiteLLM lookup fallback is intentional, add an explicit suppression rationale; otherwise narrow the catch.
Proposed fix
- except Exception:
+ except Exception: # noqa: BLE001 - litellm may raise provider-specific lookup errors here.
info = None
@@
- f"Nomic embedding generation failed: {str(e)}"
+ f"Nomic embedding generation failed: {e!s}"
@@
- f"Local embedding generation failed: {str(e)}"
+ f"Local embedding generation failed: {e!s}"
@@
- raise LiteLLMClientError(f"Embedding generation failed: {str(e)}") from e
+ raise LiteLLMClientError(f"Embedding generation failed: {e!s}") from e
@@
- f"Nomic batch embedding generation failed: {str(e)}"
+ f"Nomic batch embedding generation failed: {e!s}"
@@
- f"Local batch embedding generation failed: {str(e)}"
+ f"Local batch embedding generation failed: {e!s}"
@@
- f"Batch embedding generation failed: {str(e)}"
+ f"Batch embedding generation failed: {e!s}"Also applies to: 285-287, 305-307, 333-334, 377-379, 391-393, 421-424
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 96-96: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@reflexio/server/llm/_litellm_embedding.py` at line 96, The remaining Ruff
findings in the LiteLLM embedding module come from a broad fallback catch and
several exception messages that are not compliant. In the fallback around the
LiteLLM lookup, either narrow the bare Exception handling to the specific error
type(s) or keep it and add a clear suppression rationale for BLE001 if the catch
is intentional. Also update the exception-raising paths in the module so the
messages satisfy RUF010 by using proper exception message formatting
consistently in the affected lookup and validation branches.
Source: Linters/SAST tools
Comment on lines
+229
to
+236
| final_messages = list(messages) | ||
| if system_message: | ||
| # Check if first message is already a system message | ||
| if final_messages and final_messages[0].get("role") == "system": | ||
| # Merge with existing system message | ||
| final_messages[0]["content"] = ( | ||
| f"{system_message}\n\n{final_messages[0]['content']}" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid mutating caller-owned message dictionaries.
list(messages) only copies the list. Line 234 mutates the original first message dict when it is already a system message, so reused histories can accumulate duplicated system prompts.
🐛 Proposed fix
- final_messages = list(messages)
+ final_messages = [dict(message) for message in messages]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| final_messages = list(messages) | |
| if system_message: | |
| # Check if first message is already a system message | |
| if final_messages and final_messages[0].get("role") == "system": | |
| # Merge with existing system message | |
| final_messages[0]["content"] = ( | |
| f"{system_message}\n\n{final_messages[0]['content']}" | |
| ) | |
| final_messages = [dict(message) for message in messages] | |
| if system_message: | |
| # Check if first message is already a system message | |
| if final_messages and final_messages[0].get("role") == "system": | |
| # Merge with existing system message | |
| final_messages[0]["content"] = ( | |
| f"{system_message}\n\n{final_messages[0]['content']}" | |
| ) |
🤖 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 `@reflexio/server/llm/_litellm_text_generation.py` around lines 229 - 236,
Avoid mutating caller-owned message dictionaries in the message assembly logic.
In _litellm_text_generation.py, final_messages = list(messages) only copies the
list, so updating final_messages[0]["content"] still changes the original first
message when it is already a system message. Update the code in the
system-message merge path to copy that first message dict before modifying it,
so reused histories in final_messages and the caller’s messages stay untouched.
Comment on lines
+471
to
+484
| process.join(timeout=hard_timeout) | ||
| if process.is_alive(): | ||
| process.terminate() | ||
| process.join(timeout=1.0) | ||
| if process.is_alive(): | ||
| process.kill() | ||
| process.join(timeout=1.0) | ||
| raise LLMHardTimeoutError( | ||
| f"LLM request exceeded hard timeout of {hard_timeout:.3f}s " | ||
| f"(provider timeout={provider_timeout!r})" | ||
| ) | ||
|
|
||
| try: | ||
| status, payload = result_queue.get(timeout=1.0) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Drain the worker queue before joining the child process.
The worker writes the completion to multiprocessing.Queue; joining before reading can block on the queue feeder for large responses and turn a completed request into a false hard timeout.
🐛 Proposed direction
- process.join(timeout=hard_timeout)
- if process.is_alive():
+ try:
+ status, payload = result_queue.get(timeout=hard_timeout)
+ except queue.Empty as exc:
+ process.join(timeout=0)
+ if not process.is_alive():
+ raise LiteLLMClientError(
+ "LLM request process exited without returning a result "
+ f"(exitcode={process.exitcode})"
+ ) from exc
process.terminate()
process.join(timeout=1.0)
if process.is_alive():
process.kill()
process.join(timeout=1.0)
@@
- try:
- status, payload = result_queue.get(timeout=1.0)
- except queue.Empty as exc:
- raise LiteLLMClientError(
- "LLM request process exited without returning a result "
- f"(exitcode={process.exitcode})"
- ) from exc
+ process.join(timeout=1.0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| process.join(timeout=hard_timeout) | |
| if process.is_alive(): | |
| process.terminate() | |
| process.join(timeout=1.0) | |
| if process.is_alive(): | |
| process.kill() | |
| process.join(timeout=1.0) | |
| raise LLMHardTimeoutError( | |
| f"LLM request exceeded hard timeout of {hard_timeout:.3f}s " | |
| f"(provider timeout={provider_timeout!r})" | |
| ) | |
| try: | |
| status, payload = result_queue.get(timeout=1.0) | |
| try: | |
| status, payload = result_queue.get(timeout=hard_timeout) | |
| except queue.Empty as exc: | |
| process.join(timeout=0) | |
| if not process.is_alive(): | |
| raise LiteLLMClientError( | |
| "LLM request process exited without returning a result " | |
| f"(exitcode={process.exitcode})" | |
| ) from exc | |
| process.terminate() | |
| process.join(timeout=1.0) | |
| if process.is_alive(): | |
| process.kill() | |
| process.join(timeout=1.0) | |
| raise LLMHardTimeoutError( | |
| f"LLM request exceeded hard timeout of {hard_timeout:.3f}s " | |
| f"(provider timeout={provider_timeout!r})" | |
| ) | |
| process.join(timeout=1.0) |
🤖 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 `@reflexio/server/llm/_litellm_text_generation.py` around lines 471 - 484, The
hard-timeout handling in the LLM worker flow can falsely time out because the
child process is joined before draining result_queue. Update the logic around
the process.join and result_queue.get sequence in the text generation path so
the queue is read/drained before waiting on the child process, and keep the
timeout/kill cleanup in the same flow. Use the existing hard timeout block and
the result_queue handling in _litellm_text_generation to locate the fix.
Comment on lines
+865
to
+870
| # File path or URL | ||
| if image.startswith(("http://", "https://")): | ||
| # URL - use directly | ||
| content_blocks.append( | ||
| {"type": "image_url", "image_url": {"url": image}} # type: ignore[reportArgumentType] | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject cleartext image URLs before forwarding them.
Line 866 allows http:// image URLs. The server is not fetching them, but the LLM provider will retrieve the image over plaintext, which can expose or tamper with image content.
🛡️ Proposed fix
- if image.startswith(("http://", "https://")):
+ if image.startswith("http://"):
+ raise LiteLLMClientError(
+ "Image URLs must use HTTPS; pass bytes or a local file path instead"
+ )
+ if image.startswith("https://"):
# URL - use directly
content_blocks.append(
{"type": "image_url", "image_url": {"url": image}} # type: ignore[reportArgumentType]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # File path or URL | |
| if image.startswith(("http://", "https://")): | |
| # URL - use directly | |
| content_blocks.append( | |
| {"type": "image_url", "image_url": {"url": image}} # type: ignore[reportArgumentType] | |
| ) | |
| # File path or URL | |
| if image.startswith("http://"): | |
| raise LiteLLMClientError( | |
| "Image URLs must use HTTPS; pass bytes or a local file path instead" | |
| ) | |
| if image.startswith("https://"): | |
| # URL - use directly | |
| content_blocks.append( | |
| {"type": "image_url", "image_url": {"url": image}} # type: ignore[reportArgumentType] | |
| ) |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 865-865: Do not make http calls without encryption
Context: "http://"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🤖 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 `@reflexio/server/llm/_litellm_text_generation.py` around lines 865 - 870,
Reject plaintext image URLs in the image handling branch of
_litellm_text_generation before adding them to content_blocks. Update the
image.startswith(...) check so only secure https:// URLs are forwarded as
{"type": "image_url"} entries, and treat http:// as invalid or unsupported in
this path. Keep the change localized to the URL handling logic near
content_blocks.append and preserve the existing file-path handling for non-URL
images.
Source: Linters/SAST tools
This was referenced Jul 3, 2026
Merged
Merged
This was referenced Jul 11, 2026
This was referenced Jul 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Decompose
litellm_client.py(reflexio/server/llm/, 2058 lines, core LLM adapter, ~102 importers) into a 312-line facade + 3 concern mixins + 3 leaf modules — behavior-preserving, zero public-surface change. Final Tier-2.5 decomposition;litellm_clientstays the stable import point.Architecture (hybrid, state-driven)
LiteLLMClientis stateful, so self-bound concerns become mixins composed into the client; stateless parts become leaves:_litellm_text_generation.py(generation + hard-timeout/subprocess orchestration +_compute_cost_usd),_litellm_embedding.py(embedding + token-budget),_litellm_structured_output.py._litellm_types.py(config/response/exceptions),_litellm_json_extraction.py(6 pure fns),_litellm_subprocess.py(worker + the 6_Completion*Snapshotdataclasses).TYPE_CHECKINGstubs type the cross-mixin edges (_provider_response_format/_maybe_parse_structured_output); no shared Protocol, no cycle.Behavior-preservation safeguards (this is core LLM infra)
__all__equality, per-module cold-import, object-identity (_TRUNCATION_WARNED_MODELS+ each snapshot classisits defining module's), andhasattr(litellm_client, "litellm").import litellm+litellm.completion(...)module-attr calls; the facade retainsimport litellm+ thesuppress_debug_info/callbacks init (the ~40patch("...litellm_client.litellm.*")sites depend on the shared module object). Multiprocessing start-method unchanged (fork); snapshot pickling survives via identity.resolve_model_name,assert_provider_safe_schema) were repointed to their new module — a re-export would have made them silent no-ops running the real embedder/network.resolve_model_nameis repointed in both embedding + text-gen. Embedding tests assert mock call-counts.Test plan
tests/server/llm/488 passed / 60 skipped (incl. the identity surface guard + mock-count embedding tests); generation-service sample 375 passed;import reflexioboot OK; ruff + pyright clean; whole-branch/review-loopAPPROVE (AST byte-identity confirmed on all moved bodies; all 112 importer sites resolve).Summary by CodeRabbit
New Features
Bug Fixes