V2

[Nayjest]

No description provided.

[github-actions]

I've Reviewed the Code

🏛️ ARCHITECTURAL VIRTUOSO 🏛️
"Pluggable config loaders, a unified logging pipeline, and cleaner bootstrap elevate extensibility."

A solid 2.0 refactor that adds entry-point–driven config loaders, a RequestContext-based logging pipeline, component/bootstrap initialization, and flexible API-key checkers with tests and docs, but requires fixes for type annotations and error handling (streaming error payload, logger exceptions, Forwarded parsing), a backward-compatible alias for the renamed api_key_check field, removing the hardcoded encryption_key default, stricter config loader return validation, and minor README/.gitignore/test polish.

⚠️ 16 issues found across 32 files

#1 Missing trailing newline at end of .gitignore

.gitignore

The .gitignore file lacks a trailing newline (as indicated by the diff). This violates standard text file conventions and can cause minor tooling inconsistencies.
Tags: code-style, maintainability

#2 Missing space after blockquote markers breaks Markdown style

README.md L476,L480

In the Debugging section, two blockquote lines are missing a space after the '>' marker. While some renderers may tolerate it, this can lead to inconsistent rendering and violates common Markdown style conventions.
Tags: readability, code-style, language
Affected code:

476: >Never enable debugging mode in production environments, as it may expose sensitive information to the application logs.

Proposed change:

> Never enable debugging mode in production environments, as it may expose sensitive information to the application logs.

Affected code:

480: >Environment variables can also be defined in a `.env` file.

Proposed change:

> Environment variables can also be defined in a `.env` file.

#3 Inconsistent capitalization in bullet point

README.md L473

The second bullet under the Debugging overview starts with a lowercase word ('logging'), which is inconsistent with the prior bullet and standard sentence casing in lists.
Tags: readability, language, code-style
Affected code:

473: - logging level is set to DEBUG instead of INFO

Proposed change:

- Logging level is set to DEBUG instead of INFO

#4 Extra space in heading '### .env Files'

README.md L208

The heading for the .env files section contains two spaces after the '###', which is a typographical error and may affect tooling that parses headings.
Tags: readability, language, code-style
Affected code:

208: ###  .env Files

Proposed change:

### .env Files

#5 Return type annotations do not match actual return value (tuple returned, annotated as dict | None)

lm_proxy/api_key_check/with_request.py L22,L61

check_func and call are annotated to return dict | None, but they actually return a tuple (group, user_info). This mismatch can break callers relying on the type hints and cause runtime errors if code treats the result as a dict.
Tags: bug, maintainability
Affected code:

22:         def check_func(api_key: str) -> dict | None:

Proposed change:

        def check_func(api_key: str) -> tuple[str, Optional[dict]] | None:

Affected code:

61:     def __call__(self, api_key: str) -> dict | None:

Proposed change:

    def __call__(self, api_key: str) -> tuple[str, Optional[dict]] | None:

#6 Hardcoded default encryption key poses a security risk

lm_proxy/config.py L64-L67

The encryption_key field uses a fixed default value ("Eclipse"). If not overridden in configuration, this results in predictable encryption, undermining security. The field should be required (no default) to force explicit secure configuration.
Tags: security, bug
Affected code:

64:     encryption_key: str = Field(
65:         default="Eclipse",
66:         description="Key for encrypting sensitive data (must be explicitly set)",
67:     )

Proposed change:

    encryption_key: str = Field(
        ...,
        description="Key for encrypting sensitive data (must be explicitly set)",
    )

#7 Breaking change: renamed config field without backward-compatible alias

lm_proxy/config.py L11,L62,L78

The field was renamed from 'check_api_key' to 'api_key_check' while model_config has extra="forbid". Existing config files using 'check_api_key' will now fail validation. Add a validation alias to accept both names and update the debug_name to match the new field name.
Tags: compatibility, maintainability, naming, bug
Affected code:

11: from pydantic import BaseModel, Field, ConfigDict

Proposed change:

from pydantic import BaseModel, Field, ConfigDict, AliasChoices

Affected code:

62:     api_key_check: Union[str, Callable, dict] = Field(default="lm_proxy.core.check_api_key")

Proposed change:

    api_key_check: Union[str, Callable, dict] = Field(default="lm_proxy.core.check_api_key", validation_alias=AliasChoices("api_key_check", "check_api_key"))

Affected code:

78:             debug_name="check_api_key",

Proposed change:

            debug_name="api_key_check",

#8 Incorrect return type annotation; no validation that JSON root is a dict

lm_proxy/config_loaders/json.py L1-L6

json.load can return list, str, int, etc., but the function is annotated to return dict and does not validate the parsed value. This can mislead callers and cause downstream errors if the JSON top-level is not an object.
Tags: bug, maintainability
Affected code:

1: import json
2: 
3: 
4: def load_json_config(config_path: str) -> dict:
5:     with open(config_path, "r", encoding="utf-8") as f:
6:         return json.load(f)

Proposed change:

import json
from typing import Any, Dict


def load_json_config(config_path: str) -> Dict[str, Any]:
    with open(config_path, "r", encoding="utf-8") as f:
        data = json.load(f)
    if not isinstance(data, dict):
        raise TypeError("JSON config must be an object at the top level")
    return data

#9 Function may return None or non-dict despite annotation

lm_proxy/config_loaders/yaml.py L11-L12

yaml.safe_load can return None for empty files or non-dict types (e.g., list, scalar) if the YAML content isn't a mapping. The function is annotated to return dict, which can lead to runtime errors downstream. Enforce returning a dict by defaulting empty content to {} and validating the type.
Tags: bug, maintainability
Affected code:

11:     with open(config_path, "r", encoding="utf-8") as f:
12:         return yaml.safe_load(f)

Proposed change:

    with open(config_path, "r", encoding="utf-8") as f:
        data = yaml.safe_load(f)
    if data is None:
        return {}
    if not isinstance(data, dict):
        raise TypeError(f"YAML configuration must be a mapping (dict), got {type(data).__name__}")
    return data

#10 Incorrect error payload in streaming: passing dict to make_chunk(error=...) mangles type/message

lm_proxy/core.py L115-L117

In process_stream, the exception branch calls make_chunk(error={...}). make_chunk expects an exception object and constructs its own {message, type} from it. Passing a dict causes the resulting SSE chunk to contain error.message as the stringified dict and error.type as 'dict', which is incorrect for clients and logs.
Tags: bug
Affected code:

115:         except Exception as e:
116:             log_entry.error = e
117:             yield make_chunk(error={"message": str(e), "type": type(e).__name__})

Proposed change:

        except Exception as e:
            log_entry.error = e
            yield make_chunk(error=e)

#11 Logger exceptions can become unhandled task exceptions

lm_proxy/core.py L283-L291

log() re-raises exceptions from synchronous handlers even though it runs in a background task created by log_non_blocking, which will typically not be awaited. This leads to 'Task exception was never retrieved' and potential event loop error logs. Additionally, async handlers are scheduled with create_task without any error handling, so their exceptions are also unobserved. Logger errors should be logged but not propagated.
Tags: bug, anti-pattern, maintainability
Affected code:

283:         # check if it is async, then run both sync and async loggers in non-blocking way (sync too)
284:         if asyncio.iscoroutinefunction(handler):
285:             asyncio.create_task(handler(log_entry))
286:         else:
287:             try:
288:                 handler(log_entry)
289:             except Exception as e:
290:                 logging.error("Error in logger handler: %s", e)
291:                 raise e

Proposed change:

        # Run both sync and async loggers in a non-blocking way; never propagate logger errors
        if asyncio.iscoroutinefunction(handler):
            task = asyncio.create_task(handler(log_entry))
            task.add_done_callback(
                lambda t: logging.error("Error in async logger handler: %s", t.exception())
                if t.exception()
                else None
            )
        else:
            try:
                handler(log_entry)
            except Exception as e:
                logging.error("Error in logger handler: %s", e)

#12 Use HTTPException instead of NotImplementedError in an HTTP endpoint

lm_proxy/models_endpoint.py L7,L31-L34

Raising NotImplementedError inside an HTTP request handler will surface as a 500 Internal Server Error and can leak internal details in logs/responses. Use starlette.exceptions.HTTPException with an appropriate status code (e.g., 501) to return a proper API error response.
Tags: bug, maintainability
Affected code:

7: 

Proposed change:

from starlette.exceptions import HTTPException

Affected code:

31:                     raise NotImplementedError(
32:                         f"'{env.config.model_listing_mode}' model listing mode "
33:                         f"is not implemented yet"
34:                     )

Proposed change:

                    raise HTTPException(
                        status_code=501,
                        detail=f"'{env.config.model_listing_mode}' model listing mode is not implemented yet",
                    )

#13 resolve_instance_or_callable mutates input dict via pop

lm_proxy/utils.py L40-L42

The function modifies the provided config dict by popping the class key, which can cause unexpected side effects for callers reusing the same dict. It should operate on a copy instead.
Tags: bug, maintainability
Affected code:

40:         constructor = resolve_callable(class_name)
41:         return constructor(**item)
42:     if isinstance(item, str):

Proposed change:

        cfg = dict(item)
        class_name = cfg.pop(class_key)
        constructor = resolve_callable(class_name)
        return constructor(**cfg)

#14 Unsafe parsing of Forwarded header may raise IndexError

lm_proxy/utils.py L76-L79

get_client_ip parses the Forwarded header using split('for=')[1], which will raise IndexError if 'for=' is not present or malformed. This can trigger a 500 error on untrusted input. Use a safer parsing approach.
Tags: bug, security
Affected code:

76:     if forwarded := request.headers.get("Forwarded"):
77:         # Parse Forwarded header (RFC 7239)
78:         return forwarded.split("for=")[1].split(";")[0].strip()
79: 

Proposed change:

    if forwarded := request.headers.get("Forwarded"):
        # Parse Forwarded header (RFC 7239)
        try:
            for segment in forwarded.split(","):
                for param in segment.split(";"):
                    kv = param.strip()
                    if kv.lower().startswith("for="):
                        val = kv.split("=", 1)[1].strip().strip('"')
                        return val
        except Exception:
            pass

#15 Typographical error in test prompt: 'english' should be 'English'

tests/test_integration.py L57

The test prompt in test_streaming_response uses 'english' instead of the proper noun 'English'. This is a clear typographical error in user-facing text and should be corrected for clarity and professionalism.
Tags: readability, code-style
Affected code:

57:         "Count from 1 to 5, each number as english word (one, two, ...) on a new line",

Proposed change:

        "Count from 1 to 5, each number as English word (one, two, ...) on a new line",

#16 Test mutates global configuration without restoring it, causing cross-test side effects

tests/test_models_endpoint.py L53-L57

The test directly modifies env.config.model_listing_mode and does not restore it. This leaks global state changes to subsequent tests, leading to order-dependent and flaky test behavior.
Tags: bug, maintainability, anti-pattern
Affected code:

53:     env.config.model_listing_mode = ModelListingMode.IGNORE_WILDCARDS
54:     payload = json.loads((await models(req)).body.decode())
55:     assert len(payload["data"]) == 2  # Only 'a' and 'b'
56:     assert payload["data"][0]["id"] in ("a", "b")
57:     assert payload["data"][1]["id"] in ("a", "b")

Proposed change:

    original_mode = env.config.model_listing_mode
    try:
        env.config.model_listing_mode = ModelListingMode.IGNORE_WILDCARDS
        payload = json.loads((await models(req)).body.decode())
        assert len(payload["data"]) == 2  # Only 'a' and 'b'
        assert payload["data"][0]["id"] in ("a", "b")
        assert payload["data"][1]["id"] in ("a", "b")
    finally:
        env.config.model_listing_mode = original_mode