Conversation
WalkthroughIntroduces an async proxy registration flow with a module-level proxy registry, converts register_proxy_func to async with runtime validation, switches several Pydantic defaults to Field(default_factory=...), adds safe_error_message and wraps non-stream adapter calls to return sanitized HTTP 500 responses, and updates docs/tests to match. Changes
Sequence DiagramssequenceDiagram
actor Plugin
participant Load as framex.plugin.load
participant Registry as _PROXY_REGISTRY
participant PluginAPI as Plugin API
participant ProxyPlugin as Proxy Plugin
Plugin->>Load: await register_proxy_func(func)
activate Load
Load->>Registry: lookup func_name
alt found
Load->>PluginAPI: build PluginApi(..., ApiType.PROXY)
Load->>PluginAPI: call_plugin_api(func_name, callable)
PluginAPI->>ProxyPlugin: register function
ProxyPlugin-->>PluginAPI: ack
PluginAPI-->>Load: return
else not found
Load-->>Plugin: raise RuntimeError
end
Load-->>Plugin: return
deactivate Load
sequenceDiagram
actor Client
participant Ingress as framex.driver.ingress
participant Registry as _PROXY_REGISTRY
participant Adapter as Adapter._acall
Client->>Ingress: HTTP request to proxy endpoint
activate Ingress
Ingress->>Registry: lookup callable by name
Registry-->>Ingress: callable
Ingress->>Adapter: adapter._acall(...) wrapped in try/except
alt success
Adapter-->>Ingress: response
Ingress-->>Client: HTTP 200 with payload
else exception
Adapter-->>Ingress: raises
Ingress->>Ingress: safe_error_message(err)
Ingress-->>Client: HTTP 500 with sanitized message
end
deactivate Ingress
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/framex/plugin/on.py (1)
111-111: Cleanup: Unusedis_registeredvariable.The
is_registeredvariable is declared at line 111 and referenced withnonlocalat line 134, but it's never set or used after the removal of the lazy registration logic. This is dead code that should be removed.🔎 Proposed cleanup
if not settings.server.enable_proxy: # pragma: no cover return func - is_registered = False full_func_name = f"{func.__module__}.{func.__name__}" async def safe_callable(*args: Any, **kwargs: Any) -> Any: try: return await func(*args, **kwargs) except Exception: raw = func if isinstance(raw, (classmethod, staticmethod)): raw = raw.__func__ while hasattr(raw, "__wrapped__"): raw = raw.__wrapped__ if inspect.iscoroutinefunction(raw): return await raw(*args, **kwargs) return raw(*args, **kwargs) _PROXY_REGISTRY[full_func_name] = safe_callable @functools.wraps(func) async def wrapper(*args: Any, **kwargs: Any) -> Any: - nonlocal is_registered - if args: # pragma: no cover raise TypeError(f"The proxy function '{func.__name__}' only supports keyword arguments.")Also applies to: 134-134
🧹 Nitpick comments (3)
src/framex/utils.py (1)
152-158: LGTM: Defensive error message extraction.The
safe_error_messagefunction provides a robust fallback chain for extracting user-friendly error messages from exceptions, preventing internal implementation details from leaking through API responses.💡 Optional simplification
The
hasattr(e, "cause")check may be redundant since accessing a non-existent attribute would raiseAttributeError. Consider simplifying:def safe_error_message(e: Exception) -> str: - if hasattr(e, "cause") and e.cause: + if getattr(e, "cause", None): return str(e.cause) if e.args: return str(e.args[0]) return "Internal Server Error"This uses
getattrwith a default, which is more idiomatic Python.src/framex/plugin/load.py (1)
44-45: Consider extracting the error message to improve maintainability.The inline error message triggers a static analysis hint (TRY003). While functionally correct, extracting long messages or using a custom exception class can improve maintainability.
🔎 Optional refactor to address TRY003
+_PROXY_FUNC_NOT_REGISTERED = ( + "Function {func_name} is not registered as a proxy function." +) + async def register_proxy_func(func: Callable) -> None: full_func_name = f"{func.__module__}.{func.__name__}" if full_func_name not in _PROXY_REGISTRY: # pragma: no cover - raise RuntimeError(f"Function {full_func_name} is not registered as a proxy function.") + raise RuntimeError(_PROXY_FUNC_NOT_REGISTERED.format(func_name=full_func_name))Based on static analysis hints.
src/framex/plugins/proxy/config.py (1)
32-37: LGTM! Consider minor wording improvement.The validator correctly ensures all
proxy_functionsURLs are configured inproxy_urls, preventing runtime errors.💡 Optional: Slightly clearer error message
- raise ValueError(f"proxy_functions url '{url}' is not covered by any proxy_urls rule") + raise ValueError(f"proxy_functions url '{url}' is not in proxy_urls")The phrase "not covered by any proxy_urls rule" might suggest wildcard matching, but the check is a simple list membership test.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
book/src/advanced_usage/authentication.mdbook/src/advanced_usage/proxy_function.mdsrc/framex/config.pysrc/framex/driver/ingress.pysrc/framex/plugin/load.pysrc/framex/plugin/on.pysrc/framex/plugins/proxy/__init__.pysrc/framex/plugins/proxy/config.pysrc/framex/utils.pytests/api/test_proxy.pytests/test_plugins.py
🧰 Additional context used
🧬 Code graph analysis (5)
src/framex/plugin/load.py (2)
src/framex/plugin/model.py (3)
ApiType(22-26)Plugin(47-75)PluginApi(29-37)src/framex/plugin/__init__.py (2)
call_plugin_api(78-133)get_loaded_plugins(25-26)
src/framex/plugins/proxy/config.py (2)
src/framex/config.py (1)
AuthConfig(65-95)src/framex/plugin/__init__.py (1)
get_plugin_config(30-34)
src/framex/driver/ingress.py (3)
src/framex/utils.py (2)
escape_tag(31-33)safe_error_message(153-158)src/framex/adapter/base.py (1)
_acall(64-65)src/framex/adapter/ray_adapter.py (1)
_acall(53-54)
tests/api/test_proxy.py (3)
src/framex/utils.py (2)
cache_decode(93-132)cache_encode(67-90)tests/conftest.py (1)
client(62-64)tests/test_plugins.py (2)
ExchangeModel(160-163)SubModel(155-157)
tests/test_plugins.py (1)
src/framex/plugin/load.py (1)
register_proxy_func(42-57)
🪛 Ruff (0.14.10)
src/framex/plugin/load.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
src/framex/driver/ingress.py
106-106: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (23)
src/framex/config.py (1)
100-100: LGTM: Proper use ofField(default_factory)for mutable defaults.This change correctly avoids the mutable default argument pitfall by using
Field(default_factory=...)for the dictionary default, ensuring eachSettingsinstance gets its own independent dictionary rather than sharing a single mutable object.tests/test_plugins.py (2)
9-9: LGTM: Import aligns with new async registration flow.The import of
register_proxy_funcis necessary to explicitly register proxy functions before invocation, reflecting the shift from implicit to explicit async registration.
186-187: LGTM: Explicit async proxy registration correctly implemented.The addition of
await register_proxy_func(local_exchange_key_value)properly implements the new explicit async registration pattern. The@pytest.mark.order(1)decorator ensures this registration happens before dependent tests.src/framex/plugins/proxy/__init__.py (2)
72-76: LGTM: Enhanced error logging before raising.The addition of error logging before
raise_for_status()improves observability by capturing the status code and response text when OpenAPI docs fetching fails. This aids debugging of proxy configuration issues.
169-169: No changes needed —direct_output=Falseis correct for proxy function route registration.The change from
direct_output=Truetodirect_output=Falsealigns with the intended response handling pattern. The proxy function route (_proxy_func_route) returns a string fromcall_proxy_function, which requires standard response wrapping through the ingress layer. This differs from the proxy API endpoint (line 143) which usesdirect_output=Trueto pass raw external API responses directly. Thedirect_output=Falsesetting ensures consistent response processing for proxy function results.book/src/advanced_usage/authentication.md (2)
64-64: LGTM: Cosmetic formatting improvement.The change to inline TOML format for the
rulesmapping is purely presentational. The semantic meaning remains identical while improving readability for simple key-value pairs.
85-85: LGTM: Consistent formatting with API auth configuration.The inline format matches the style used in the API authentication section, providing consistency across the documentation.
src/framex/driver/ingress.py (2)
19-19: LGTM: Import supports new error sanitization pattern.The
safe_error_messageimport is used to sanitize exception details before exposing them in HTTP 500 responses, improving security and consistency.
104-110: LGTM: Appropriate error handling at API boundary.The try-except block properly converts any exception from
adapter._acallinto an HTTP 500 response with a sanitized error message. The broad exception catch is appropriate here as this is the API ingress boundary where all internal errors should be caught and converted to proper HTTP responses.The
from Nonesuppression of the exception chain is reasonable for API responses to avoid leaking internal implementation details.tests/api/test_proxy.py (3)
62-70: LGTM: Improved test structure with explicit status check.The refactored test now explicitly validates the response status before decoding the data, making the test more robust and aligned with the new response structure that includes both status and data fields.
73-86: LGTM: Good coverage of error scenario.The new test validates that calling a non-existent proxy function returns HTTP 500, which exercises the error handling path introduced in
src/framex/driver/ingress.pywith thesafe_error_messagesanitization.
89-102: LGTM: Authentication error handling validated.The test correctly validates that an invalid API key results in HTTP 401, ensuring the authentication layer is properly enforced for proxy function calls.
src/framex/plugin/on.py (3)
101-101: LGTM: Centralized proxy function registry introduced.The module-level
_PROXY_REGISTRYprovides a centralized store for proxy functions, enabling the explicit async registration pattern implemented elsewhere in the PR.
130-130: LGTM: Function registered in centralized registry.Storing the
safe_callablewrapper in_PROXY_REGISTRYenables external registration viaregister_proxy_func(insrc/framex/plugin/load.py) while keeping the original function's unwrapping logic intact.
144-150: LGTM: Clean proxy invocation via PluginApi.The wrapper now delegates to
call_proxy_functionthrough the PluginApi, properly encoding the function name and arguments. This aligns with the shift from implicit lazy registration to explicit async registration.src/framex/plugin/load.py (3)
3-8: LGTM!All imports are correctly used in the new async
register_proxy_funcimplementation.
42-43: LGTM!The function signature correctly transitions to async, and the fully-qualified name construction is appropriate for registry lookup.
47-57: The parameter names are correct. Thefunc_nameandfunc_callablekeyword arguments match theregister_proxy_functionsignature exactly:async def register_proxy_function( self, func_name: str, func_callable: Callable[..., Any], is_remote: bool = False ) -> bool:The optional
is_remoteparameter is not passed, which is acceptable since it has a default value.book/src/advanced_usage/proxy_function.md (3)
49-49: LGTM!Documentation correctly reflects the async
register_proxy_funcAPI, withawaitused in the appropriate async context.
76-76: LGTM!The updated example URL clarifies remote invocation and is consistent with the
proxy_urlsconfiguration on line 74, satisfying the validation inProxyPluginConfig.
83-83: LGTM!The authentication configuration correctly demonstrates the per-path
rulesmapping, matching theAuthConfig.rulesstructure fromsrc/framex/config.py.src/framex/plugins/proxy/config.py (2)
3-3: LGTM!The Pydantic v2 imports (
Field,model_validator) are correctly used throughout the file for default factories and validation.
10-19: LGTM!All mutable defaults correctly migrated to
Field(default_factory=...), following Pydantic v2 best practices to prevent shared references between instances.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/framex/plugins/proxy/__init__.py (1)
255-257: Consider using safe_error_message for consistent error handling.The hardcoded
"error proxy"return value is inconsistent with the newsafe_error_messageutility introduced in this PR. Consider using the utility to provide more helpful error messages to callers while still sanitizing sensitive information.🔎 Proposed refactor to use safe_error_message
First, import the utility at the top of the file:
+from framex.utils import cache_decode, cache_encode, safe_error_message -from framex.utils import cache_decode, cache_encodeThen update the error handling:
except Exception as e: logger.opt(exception=e, colors=True).error(f"Error calling proxy api({method}) <y>{url}</y>: {e}") - return "error proxy" + return {"error": safe_error_message(e)}tests/test_utils.py (1)
164-183: Consider supporting standard Python exception chaining.The implementation only checks a custom
causeattribute but ignores the standard__cause__attribute set byraise ... from ...syntax (PEP 3134). The codebase uses standard exception chaining insrc/framex/plugin/__init__.py, sosafe_error_messagemay return incomplete error information in error responses.Suggested enhancement:
def safe_error_message(e: Exception) -> str: # Check custom cause attribute first (for backward compatibility) if hasattr(e, "cause") and e.cause: return str(e.cause) # Check standard __cause__ from exception chaining if e.__cause__: return str(e.__cause__) if e.args: return str(e.args[0]) return "Internal Server Error"Add test:
def test_safe_error_message_with_standard_cause(): try: try: raise ValueError("inner error") except ValueError as inner: raise RuntimeError("outer error") from inner except RuntimeError as e: assert safe_error_message(e) == "inner error"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/framex/plugins/proxy/__init__.pytests/test_utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_utils.py (1)
src/framex/utils.py (1)
safe_error_message(153-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
src/framex/plugins/proxy/__init__.py (2)
8-8: LGTM: Import added for HTTP status constants.The import is used appropriately for the defensive status check in
_get_openai_docs.
170-170: Thedirect_output=Falsechange is correct and properly tested.The tests in
tests/api/test_proxy.pyconfirm that HTTP clients calling/api/v1/proxy/remoteexpect wrapped responses with{"status": ...}fields. Withdirect_output=False, the framework properly wraps responses and handles errors—as verified by test cases that expectstatuscodes (200 for success, 500 for runtime errors like unregistered functions). This is the correct behavior for HTTP endpoints.tests/test_utils.py (1)
9-16: LGTM: Import updated correctly.The
safe_error_messageimport is added appropriately alongside existing utility imports.
| if response.status_code != status.HTTP_200_OK: # pragma: no cover | ||
| logger.error( | ||
| f"Failed to get openai docs from {url}, status code: {response.status_code}, response: {response.text}" | ||
| ) |
There was a problem hiding this comment.
Potential information disclosure in error logging.
Logging the full response.text in error messages could expose sensitive information if the remote service returns detailed error responses (stack traces, internal paths, configuration details, etc.). Consider sanitizing or truncating the response text, or using the safe_error_message utility introduced in this PR.
🔎 Proposed fix to limit logged response content
if response.status_code != status.HTTP_200_OK: # pragma: no cover
logger.error(
- f"Failed to get openai docs from {url}, status code: {response.status_code}, response: {response.text}"
+ f"Failed to get openai docs from {url}, status code: {response.status_code}, response: {response.text[:200]}"
)📝 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.
| if response.status_code != status.HTTP_200_OK: # pragma: no cover | |
| logger.error( | |
| f"Failed to get openai docs from {url}, status code: {response.status_code}, response: {response.text}" | |
| ) | |
| if response.status_code != status.HTTP_200_OK: # pragma: no cover | |
| logger.error( | |
| f"Failed to get openai docs from {url}, status code: {response.status_code}, response: {response.text[:200]}" | |
| ) |
🤖 Prompt for AI Agents
In src/framex/plugins/proxy/__init__.py around lines 73 to 76, the error log
currently includes the full response.text which may leak sensitive data; replace
that direct inclusion with a sanitized/truncated value by using the
safe_error_message utility (or implement a short sanitizer) and log only the
sanitized message along with url and status code; if safe_error_message is not
imported in this file, add the import and ensure the sanitizer trims long bodies
and strips sensitive-looking content before passing to logger.error.
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.