Conversation
WalkthroughAdds a proxy function system: decorator-based registration and invocation routed through a new HTTP proxy endpoint, plus encode/decode caching utilities, config/constant updates, deferred auth/setting resolution, and accompanying tests and mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Decorator as on_proxy
participant Cache as cache_encode/cache_decode
participant PluginAPI as call_plugin_api
participant ProxyPlugin
rect rgb(230,240,255)
note over Client,ProxyPlugin: Registration (first call)
Client->>Decorator: call decorated(...)
Decorator->>Cache: cache_encode(func_name, kwargs)
Cache-->>Decorator: encoded_payload
Decorator->>PluginAPI: call_plugin_api(PROXY_PLUGIN_NAME, register_proxy_function, func_callable)
PluginAPI->>ProxyPlugin: register_proxy_function(func_name, callable)
ProxyPlugin-->>PluginAPI: status
PluginAPI-->>Decorator: result
Decorator-->>Client: return
end
rect rgb(230,255,230)
note over Client,ProxyPlugin: Invocation (subsequent calls)
Client->>Decorator: call decorated(...)
Decorator->>Cache: cache_encode(func_name, kwargs)
Cache-->>Decorator: encoded_payload
Decorator->>PluginAPI: call_plugin_api(PROXY_PLUGIN_NAME, call_proxy_function, encoded_payload)
PluginAPI->>ProxyPlugin: call_proxy_function(func_name, data)
alt function is_remote
ProxyPlugin->>ProxyPlugin: POST /proxy/remote (auth + data)
ProxyPlugin-->>PluginAPI: serialized_result
else local
ProxyPlugin->>ProxyPlugin: invoke local callable
ProxyPlugin-->>PluginAPI: serialized_result
end
PluginAPI-->>Decorator: serialized_result
Decorator->>Cache: cache_decode(serialized_result)
Cache-->>Decorator: decoded_result
Decorator-->>Client: return decoded_result
end
sequenceDiagram
participant TestClient
participant MockServer
participant ProxyEndpoint as /api/v1/proxy/remote
participant Cache as cache_decode
TestClient->>MockServer: POST /api/v1/proxy/remote (Authorization + payload)
MockServer->>ProxyEndpoint: route request
ProxyEndpoint->>Cache: cache_decode(func_name)
Cache-->>ProxyEndpoint: func_name
ProxyEndpoint->>Cache: cache_decode(data)
Cache-->>ProxyEndpoint: kwargs
alt auth valid
ProxyEndpoint->>ProxyEndpoint: execute target (local or simulated remote)
ProxyEndpoint-->>TestClient: {"result":..., "data": cache_encode(returned)}
else auth invalid
ProxyEndpoint-->>TestClient: 401-like response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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❌ Patch coverage is 📢 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/framex/plugins/proxy/__init__.py (2)
65-69: httpx.AsyncClient not closed properly.The
AsyncClientis created but never closed, which can leak connections. Use a context manager or explicitly close the client.🔎 Proposed fix
async def _get_openai_docs(self, url: str) -> dict[str, Any]: - clent = httpx.AsyncClient(timeout=self.time_out) - response = await clent.get(f"{url}/api/v1/openapi.json") - response.raise_for_status() - return cast(dict[str, Any], response.json()) + async with httpx.AsyncClient(timeout=self.time_out) as client: + response = await client.get(f"{url}/api/v1/openapi.json") + response.raise_for_status() + return cast(dict[str, Any], response.json())
191-201: httpx.AsyncClient not closed in non-streaming path.Similar to
_get_openai_docs, the client created on line 191 is not closed after use in the non-streaming branch (line 201).🔎 Proposed fix
async def fetch_response( self, stream: bool = False, **kwargs: Any, ) -> AsyncGenerator[str, None] | dict | str: - clent = httpx.AsyncClient(timeout=self.time_out) if stream: + client = httpx.AsyncClient(timeout=self.time_out) async def stream_generator() -> AsyncGenerator[str, None]: - async with clent.stream(**kwargs) as response: + async with client.stream(**kwargs) as response: response.raise_for_status() async for chunk in response.aiter_text(): yield chunk + await client.aclose() return stream_generator() - response = await clent.request(**kwargs) - response.raise_for_status() - try: - return cast(dict, response.json()) - except json.JSONDecodeError: - return response.text + async with httpx.AsyncClient(timeout=self.time_out) as client: + response = await client.request(**kwargs) + response.raise_for_status() + try: + return cast(dict, response.json()) + except json.JSONDecodeError: + return response.text
🧹 Nitpick comments (5)
src/framex/plugins/proxy/config.py (1)
32-37: LGTM! Validation ensures consistency.The validator correctly ensures that every proxy function URL is covered by a proxy_urls entry, preventing configuration errors.
Optional: Consider extracting the error message to a constant
The static analysis tool suggests avoiding long messages in exception constructors. While this is a minor style concern, you could extract it if desired:
+ _INVALID_PROXY_URL_MSG = "proxy_functions url '{url}' is not covered by any proxy_urls rule" + @model_validator(mode="after") def validate_proxy_functions(self) -> Self: for url in self.proxy_functions: if url not in self.proxy_urls: - raise ValueError(f"proxy_functions url '{url}' is not covered by any proxy_urls rule") + raise ValueError(self._INVALID_PROXY_URL_MSG.format(url=url)) return selfsrc/framex/plugin/__init__.py (1)
120-123: Remove commented code.The commented-out warning should be removed rather than left in the codebase.
Proposed cleanup
if use_proxy: if not isinstance(result, dict): - # logger.warning(f"Proxy API {api_name} returned non-dict result: {type(result)}") return resultsrc/framex/utils.py (3)
61-62: Document that XOR is obfuscation, not encryption.The
xor_cryptfunction uses a fixed key for XOR operations, which provides obfuscation but not cryptographic security. If this is intentional (e.g., to prevent casual inspection of cached data), please add a docstring clarifying this is not secure encryption.Suggested docstring
def xor_crypt(data: bytes, key: str = "01234567890abcdefghijklmnopqrstuvwxyz") -> bytes: + """Apply XOR-based obfuscation to data. + + Note: This provides basic obfuscation only, NOT cryptographic security. + Do not rely on this for protecting sensitive data. + """ return bytes(a ^ b for a, b in zip(data, cycle(key.encode())))
65-77: Add error handling for malformed input.The
cache_encodeandcache_decodefunctions lack error handling for malformed input. Invalid hex strings, compression errors, or unpickling failures will raise unhandled exceptions.Suggested error handling
def cache_decode(data: str) -> Any: - raw = bytes.fromhex(data) - de_obfuscated = xor_crypt(raw) - decompressed = zlib.decompress(de_obfuscated) - return cloudpickle.loads(decompressed) # nosec S301 + try: + raw = bytes.fromhex(data) + de_obfuscated = xor_crypt(raw) + decompressed = zlib.decompress(de_obfuscated) + return cloudpickle.loads(decompressed) # nosec S301 + except (ValueError, zlib.error, Exception) as e: + raise ValueError(f"Failed to decode cached data: {e}") from e
61-77: Consider adding docstrings for new utility functions.The new encoding/decoding utilities lack documentation explaining their purpose, parameters, and return values, which would improve maintainability.
📜 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 (12)
pyproject.tomlpytest.inisrc/framex/adapter/base.pysrc/framex/plugin/__init__.pysrc/framex/plugin/on.pysrc/framex/plugins/proxy/__init__.pysrc/framex/plugins/proxy/config.pysrc/framex/plugins/proxy/model.pysrc/framex/utils.pytests/mock.pytests/test_plugin.pytests/test_plugins.py
💤 Files with no reviewable changes (1)
- tests/test_plugin.py
🧰 Additional context used
🧬 Code graph analysis (5)
tests/mock.py (2)
src/framex/cli.py (1)
framex(11-12)src/framex/utils.py (1)
cache_decode(73-77)
src/framex/plugin/on.py (3)
src/framex/plugin/model.py (3)
ApiType(22-26)PluginApi(29-37)PluginDeployment(41-43)src/framex/utils.py (4)
cache_decode(73-77)cache_encode(65-70)extract_method_params(34-41)plugin_to_deployment_name(15-16)src/framex/plugin/__init__.py (1)
call_plugin_api(78-134)
src/framex/plugin/__init__.py (2)
src/framex/plugin/model.py (1)
PluginApi(29-37)src/framex/plugin/manage.py (1)
get_api(196-201)
src/framex/adapter/base.py (1)
src/framex/plugin/model.py (1)
ApiType(22-26)
tests/test_plugins.py (1)
src/framex/plugin/on.py (1)
on_proxy(101-138)
🪛 Ruff (0.14.8)
src/framex/plugin/on.py
116-119: Avoid specifying long messages outside the exception class
(TRY003)
src/framex/plugins/proxy/__init__.py
283-283: Avoid specifying long messages outside the exception class
(TRY003)
src/framex/plugins/proxy/config.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_plugins.py
171-171: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (20)
pytest.ini (1)
30-30: LGTM! Test configuration properly wired.The proxy function mapping correctly links the test proxy URL to the test function, aligning with the new proxy function infrastructure.
src/framex/adapter/base.py (1)
32-34: LGTM! Conditional proxy handling is appropriate.The added check ensures
proxy_pathis only set when an actual API endpoint exists. This correctly accommodates the new proxy function feature where proxy calls may occur without explicit API paths.src/framex/plugins/proxy/config.py (1)
19-19: LGTM! New field properly typed.The
proxy_functionsfield correctly maps proxy URLs to lists of function names, supporting the new proxy function registration feature.src/framex/plugins/proxy/model.py (2)
7-9: LGTM! Model structure is clean.The
ProxyFuncmodel appropriately captures function metadata. Note that Callable fields don't serialize with Pydantic's standard JSON serialization, but this is expected for in-memory proxy function registration.
12-14: LGTM! HTTP body model is appropriate.The
ProxyFuncHttpBodymodel correctly defines the structure for transporting encoded proxy function invocations.src/framex/plugin/__init__.py (1)
78-86: LGTM! Enhanced API calling is more flexible.The updated signature correctly supports both string API names and direct PluginApi instances, enabling the proxy function feature to pass pre-constructed API objects.
pyproject.toml (1)
11-11: LGTM! Dependency required for proxy function serialization.The
cloudpickledependency is correctly added to support the new encoding/decoding utilities for proxy function payloads.src/framex/utils.py (1)
51-58: LGTM! BaseModel handling is correctly implemented.The expanded signature and BaseModel serialization logic properly supports streaming events with Pydantic models.
tests/test_plugins.py (5)
8-9: LGTM!Import of the new
on_proxydecorator is correctly added for testing the proxy functionality.
153-161: LGTM!The
SubModelandExchangeModelPydantic models are well-defined with proper type annotations for testing the proxy function serialization with nested models.
169-171: LGTM on the remote proxy function definition.The
proxy_only=Trueparameter correctly indicates this function should only be invoked remotely. TheRuntimeErrorserves as a safeguard if local invocation occurs unexpectedly.
174-183: LGTM!The test correctly verifies local proxy function invocation, including proper serialization/deserialization of nested Pydantic models. The assertions validate both primitive types and complex model structures.
186-194: LGTM!The remote proxy test verifies that
proxy_only=Truefunctions are routed correctly and return the expected response structure with the function name and data.src/framex/plugin/on.py (2)
1-1: LGTM!The new imports are correctly added to support the
on_proxydecorator functionality:functoolsfor@wraps,PROXY_PLUGIN_NAMEconstant, encoding utilities, andcall_plugin_api.Also applies to: 10-10, 12-12, 14-14
128-134: LGTM on the proxy call logic.The encoding of both function name and kwargs using
cache_encode, along with the subsequentcache_decodeof the result, correctly handles the serialization round-trip for proxy invocation.src/framex/plugins/proxy/__init__.py (5)
19-20: LGTM!Imports for the new proxy function models and encoding utilities are correctly added.
38-41: LGTM!New instance attributes properly initialize the proxy function map, route initialization flag, and HTTP path.
142-163: LGTM!The
register_proxy_func_routemethod correctly sets up the HTTP route for proxy function invocation with the appropriate parameters and tags.
165-179: LGTM!The
_parse_proxy_functionmethod properly configures remote proxy functions with authentication headers when required and registers them as remote handlers.
272-283: LGTM on the proxy function invocation logic.The
call_proxy_functionmethod correctly handles both remote and local invocations, decoding the function name, selecting the appropriate kwargs format, and encoding the result.
| def on_proxy(proxy_only: bool = False) -> Callable: | ||
| def decorator(func: Callable) -> Callable: | ||
| from framex.config import settings | ||
|
|
||
| if not settings.server.enable_proxy: | ||
| return func |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the on.py file and examine the on_proxy decorator
find . -name "on.py" -path "*/plugin/*" -type f | head -5Repository: touale/FrameX-kit
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Get more context about the on_proxy decorator and enable_proxy
rg -n "on_proxy|enable_proxy|proxy_only" --type py -B2 -A5 | head -100Repository: touale/FrameX-kit
Length of output: 6367
🏁 Script executed:
#!/bin/bash
# Read the on_proxy decorator implementation with full context
cat -n src/framex/plugin/on.py | head -150Repository: touale/FrameX-kit
Length of output: 6670
🏁 Script executed:
#!/bin/bash
# Check if there's a test for proxy_only=True with enable_proxy=False
rg -n "proxy_only.*True" tests/ -A10 -B5 | grep -E "(enable_proxy|test_|def )" | head -30Repository: touale/FrameX-kit
Length of output: 1114
🏁 Script executed:
#!/bin/bash
# Check for any test that covers proxy_only=True with enable_proxy=False
rg -n "proxy_only.*True" tests/ --type py -B15 | grep -E "(def test_|enable_proxy.*False|patch.*enable_proxy)"Repository: touale/FrameX-kit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look at the full test_on_proxy_remote_call test to see how it's tested
sed -n '186,200p' tests/test_plugins.pyRepository: touale/FrameX-kit
Length of output: 513
🏁 Script executed:
#!/bin/bash
# Check test file for any test with proxy_only=True and enable_proxy=False
cat -n tests/test_plugins.py | grep -A30 "def test_on_proxy"Repository: touale/FrameX-kit
Length of output: 1088
🏁 Script executed:
#!/bin/bash
# Look for any handling of proxy_only in the entire codebase
rg -n "proxy_only" --type py -B5 -A10Repository: touale/FrameX-kit
Length of output: 2471
🏁 Script executed:
#!/bin/bash
# Check if there are any other proxy-related functions with proxy_only=True
rg -n "@on_proxy\(proxy_only" --type pyRepository: touale/FrameX-kit
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Look at how enable_proxy is typically used in tests and if there's a default behavior
rg -n "enable_proxy" tests/ --type py | head -20Repository: touale/FrameX-kit
Length of output: 731
🏁 Script executed:
#!/bin/bash
# Check if enable_proxy has a default value in settings
rg -n "enable_proxy" src/framex/config.py --type py -B3 -A3Repository: touale/FrameX-kit
Length of output: 308
🏁 Script executed:
#!/bin/bash
# Look for any documentation or comments about proxy_only parameter
rg -n "proxy_only" . --type py -B5 | head -40Repository: touale/FrameX-kit
Length of output: 1029
🏁 Script executed:
#!/bin/bash
# Check for pytest conftest that might set default enable_proxy
find tests -name "conftest.py" -type f -exec cat {} \;Repository: touale/FrameX-kit
Length of output: 1802
🏁 Script executed:
#!/bin/bash
# Check if test_on_proxy_remote_call is being skipped or has fixtures that patch enable_proxy
sed -n '160,210p' tests/test_plugins.pyRepository: touale/FrameX-kit
Length of output: 1342
🏁 Script executed:
#!/bin/bash
# Check if framex.run with test_mode=True sets enable_proxy
rg -n "test_mode" src/ --type py -B3 -A10 | head -50Repository: touale/FrameX-kit
Length of output: 1838
🏁 Script executed:
#!/bin/bash
# Check the full test setup and whether enable_proxy is patched for test_on_proxy_remote_call
sed -n '1,30p' tests/test_plugins.pyRepository: touale/FrameX-kit
Length of output: 1363
🏁 Script executed:
#!/bin/bash
# Check if the test fixture patches enable_proxy or if proxy is enabled in test mode
rg -n "enable_proxy" tests/test_plugins.py --type py -B5 -A5Repository: touale/FrameX-kit
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look at full test setup to understand how test_on_proxy_remote_call actually works
sed -n '1,100p' tests/conftest.py | cat -nRepository: touale/FrameX-kit
Length of output: 2292
🏁 Script executed:
#!/bin/bash
# Check if test_on_proxy_remote_call has any pytest marks or fixtures
sed -n '160,196p' tests/test_plugins.py | cat -nRepository: touale/FrameX-kit
Length of output: 1594
🏁 Script executed:
#!/bin/bash
# Check entire test_plugins.py for any imports or fixtures that might enable proxy
head -50 tests/test_plugins.py | cat -nRepository: touale/FrameX-kit
Length of output: 2361
🏁 Script executed:
#!/bin/bash
# Check if there's a pytest.ini or setup that might configure enable_proxy
cat pytest.ini pyproject.toml 2>/dev/null | grep -A20 -B5 "pytest\|enable_proxy" || echo "No pytest config found"Repository: touale/FrameX-kit
Length of output: 2061
🏁 Script executed:
#!/bin/bash
# Check if framex.run or test_mode enables proxy by default
rg -n "enable_proxy.*=" src/framex/ --type py -B2 -A2 | head -30Repository: touale/FrameX-kit
Length of output: 1040
🏁 Script executed:
#!/bin/bash
# Check if there's any validation or handling for proxy_only when enable_proxy is False
rg -n "proxy_only" src/ --type py -B5 -A10Repository: touale/FrameX-kit
Length of output: 1414
🏁 Script executed:
#!/bin/bash
# Look at the full on_proxy decorator once more to ensure we understand all code paths
sed -n '101,139p' src/framex/plugin/on.py | cat -nRepository: touale/FrameX-kit
Length of output: 1819
Add defensive handling for proxy_only=True when proxy is disabled.
When proxy_only=True but settings.server.enable_proxy=False, the decorator returns the original function unchanged. For functions designed to run only through the proxy (like remote_exchange_key_value), this causes an unexpected RuntimeError instead of surfacing a clear configuration error.
Consider either:
- Raising a clear configuration error during decoration when
proxy_only=Trueand proxy is disabled - Returning a stub function that provides a helpful error message explaining the misconfiguration
🤖 Prompt for AI Agents
In src/framex/plugin/on.py around lines 101 to 106, the on_proxy decorator
silently returns the original function when settings.server.enable_proxy is
False, which causes functions intended to be proxy-only to fail later with a
confusing RuntimeError; update the decorator so that when proxy_only=True and
enable_proxy is False it either raises a clear ConfigurationError (preferred) at
decoration time or replaces the function with a stub that immediately raises a
descriptive error indicating that the proxy is disabled and the function
requires enable_proxy=True; implement one of these behaviors and include the
function name in the error message for clarity.
| is_registered = proxy_only | ||
| full_func_name = f"{func.__module__}.{func.__name__}" | ||
|
|
||
| @functools.wraps(func) | ||
| async def wrapper(*args: Any, **kwargs: Any) -> Any: | ||
| nonlocal is_registered | ||
|
|
||
| if args: | ||
| raise TypeError( | ||
| f"The proxy function '{func.__name__}' only supports keyword arguments. " | ||
| f"Please call it using the syntax {func.__name__}(param=value)." | ||
| ) | ||
|
|
||
| if not is_registered: | ||
| api_reg = PluginApi( | ||
| deployment_name=PROXY_PLUGIN_NAME, call_type=ApiType.PROXY, func_name="register_proxy_function" | ||
| ) | ||
| await call_plugin_api(api_reg, None, func_name=full_func_name, func_callable=func) | ||
| is_registered = True |
There was a problem hiding this comment.
Race condition on is_registered in concurrent scenarios.
The nonlocal is_registered flag is checked and set without synchronization. If multiple coroutines call the same proxy function concurrently before registration completes, multiple registration attempts could occur.
While duplicate registrations may be idempotent, consider using an asyncio.Lock if registration has side effects or costs.
🔎 Proposed fix using asyncio.Lock
+import asyncio
+
def on_proxy(proxy_only: bool = False) -> Callable:
def decorator(func: Callable) -> Callable:
from framex.config import settings
if not settings.server.enable_proxy:
return func
is_registered = proxy_only
full_func_name = f"{func.__module__}.{func.__name__}"
+ registration_lock = asyncio.Lock()
@functools.wraps(func)
async def wrapper(*args: Any, **kwargs: Any) -> Any:
nonlocal is_registered
if args:
raise TypeError(
f"The proxy function '{func.__name__}' only supports keyword arguments. "
f"Please call it using the syntax {func.__name__}(param=value)."
)
if not is_registered:
- api_reg = PluginApi(
- deployment_name=PROXY_PLUGIN_NAME, call_type=ApiType.PROXY, func_name="register_proxy_function"
- )
- await call_plugin_api(api_reg, None, func_name=full_func_name, func_callable=func)
- is_registered = True
+ async with registration_lock:
+ if not is_registered: # Double-check after acquiring lock
+ api_reg = PluginApi(
+ deployment_name=PROXY_PLUGIN_NAME, call_type=ApiType.PROXY, func_name="register_proxy_function"
+ )
+ await call_plugin_api(api_reg, None, func_name=full_func_name, func_callable=func)
+ is_registered = TrueCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
116-119: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In src/framex/plugin/on.py around lines 108 to 126, the nonlocal boolean
is_registered is read and set without synchronization which can cause concurrent
coroutines to trigger multiple registrations; introduce an asyncio.Lock defined
in the outer scope (next to is_registered), mark it nonlocal in the wrapper, and
wrap the check-and-register sequence in "async with lock:" so only one coroutine
performs the registration: inside the lock re-check is_registered, call
PluginApi/register only if still False, then set is_registered = True; other
calls awaiting the lock will see the updated flag and skip redundant
registration.
| async def register_proxy_function( | ||
| self, func_name: str, func_callable: Callable[..., Any], is_remote: bool = False | ||
| ) -> bool: | ||
| if not self.init_proxy_func_route: | ||
| await self.register_proxy_func_route() | ||
| self.init_proxy_func_route = True | ||
|
|
||
| logger.info(f"Registering proxy function: {func_name}") | ||
| self.proxy_func_map[func_name] = ProxyFunc(func=func_callable, is_remote=is_remote) | ||
| return True |
There was a problem hiding this comment.
Race condition on init_proxy_func_route flag.
Similar to the is_registered issue in on.py, concurrent calls to register_proxy_function could trigger multiple route registrations before the flag is set. Consider using a lock or moving route registration to on_start.
🔎 Proposed fix using asyncio.Lock
Add a lock in __init__:
self._route_registration_lock = asyncio.Lock()Then use it:
async def register_proxy_function(
self, func_name: str, func_callable: Callable[..., Any], is_remote: bool = False
) -> bool:
- if not self.init_proxy_func_route:
- await self.register_proxy_func_route()
- self.init_proxy_func_route = True
+ if not self.init_proxy_func_route:
+ async with self._route_registration_lock:
+ if not self.init_proxy_func_route:
+ await self.register_proxy_func_route()
+ self.init_proxy_func_route = True
logger.info(f"Registering proxy function: {func_name}")
self.proxy_func_map[func_name] = ProxyFunc(func=func_callable, is_remote=is_remote)
return TrueCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/framex/plugin/on.py (1)
108-126: Note: Race condition on registration flag.The
is_registeredflag is checked and set without synchronization (lines 121-126). While duplicate registrations may be idempotent, concurrent calls to the same proxy function before registration completes could trigger multiple registration attempts.This issue was previously identified in past reviews. If registration has side effects or costs, consider using an
asyncio.Lockto synchronize the check-and-register sequence.tests/test_plugins.py (1)
164-166: Typo fixed from previous review.The function name has been corrected from
lcoal_exchange_key_valuetolocal_exchange_key_value. Implementation looks good for testing local proxy execution.src/framex/plugins/proxy/__init__.py (2)
52-57: Incorrectfor-elselogic persists from previous review.The
elseclause on line 56 executes when theforloop completes normally (withoutbreak), so"No proxy functions to register"will be logged even when functions are successfully registered. This was flagged in a previous review but remains unfixed.🔎 Proposed fix
- if settings.proxy_functions: - for url, funcs in settings.proxy_functions.items(): - for func in funcs: - await self._parse_proxy_function(func, url) - else: # pragma: no cover + if settings.proxy_functions: + for url, funcs in settings.proxy_functions.items(): + for func in funcs: + await self._parse_proxy_function(func, url) + else: logger.debug("No proxy functions to register")
263-275: Race condition on route registration persists from previous review.Lines 266-268 have a time-of-check-to-time-of-use (TOCTOU) race: concurrent calls can both see
init_proxy_func_route == Falseand attempt to register the route multiple times before the flag is set. While the duplicate function check (lines 269-271) prevents function re-registration, the route registration itself could still be invoked concurrently. This was flagged in a previous review suggesting anasyncio.Lock.
🧹 Nitpick comments (2)
pytest.ini (1)
36-37: Remove trailing empty lines.Minor style issue: trailing empty lines at the end of the file.
🔎 Proposed fix
auth__general_auth_keys=["i_am_general_auth_keys"] auth__auth_urls=["/api/v1/echo"] - - + asyncio_mode = autosrc/framex/plugins/proxy/__init__.py (1)
277-289: LGTM: Proxy function routing logic is correct.The method properly decodes parameters, routes between local and remote execution paths based on the
is_remoteflag, and encodes the result. The logic for constructing kwargs for remote vs. local calls is sound.Note: Static analysis flagged the error message on line 289 (TRY003). If desired, you could define a custom exception class, but this is a minor stylistic preference.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pytest.inisrc/framex/config.pysrc/framex/consts.pysrc/framex/driver/ingress.pysrc/framex/log.pysrc/framex/plugin/__init__.pysrc/framex/plugin/on.pysrc/framex/plugins/proxy/__init__.pysrc/framex/plugins/proxy/config.pytests/mock.pytests/test_plugins.py
🧰 Additional context used
🧬 Code graph analysis (6)
tests/test_plugins.py (1)
src/framex/plugin/on.py (1)
on_proxy(101-138)
src/framex/driver/ingress.py (1)
src/framex/config.py (1)
get_auth_keys(86-110)
src/framex/plugins/proxy/__init__.py (5)
src/framex/plugin/model.py (2)
PluginApi(29-37)ApiType(22-26)src/framex/plugins/proxy/builder.py (1)
create_pydantic_model(70-93)src/framex/plugins/proxy/model.py (2)
ProxyFunc(7-9)ProxyFuncHttpBody(12-14)src/framex/utils.py (2)
cache_decode(73-77)cache_encode(65-70)src/framex/adapter/base.py (3)
BaseAdapter(20-68)get_handle(53-53)call_func(29-39)
src/framex/plugin/on.py (3)
src/framex/plugin/model.py (2)
ApiType(22-26)PluginApi(29-37)src/framex/utils.py (4)
cache_decode(73-77)cache_encode(65-70)extract_method_params(34-41)plugin_to_deployment_name(15-16)src/framex/plugin/__init__.py (1)
call_plugin_api(78-133)
src/framex/plugin/__init__.py (2)
src/framex/plugin/model.py (1)
PluginApi(29-37)src/framex/plugin/manage.py (1)
get_api(196-201)
tests/mock.py (1)
src/framex/utils.py (1)
cache_decode(73-77)
🪛 Ruff (0.14.10)
tests/test_plugins.py
171-171: Avoid specifying long messages outside the exception class
(TRY003)
src/framex/plugins/proxy/config.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
src/framex/plugins/proxy/__init__.py
289-289: Avoid specifying long messages outside the exception class
(TRY003)
src/framex/plugin/on.py
116-119: Avoid specifying long messages outside the exception class
(TRY003)
tests/mock.py
79-79: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (26)
src/framex/consts.py (1)
8-9: LGTM!The new proxy constants are well-defined and follow the existing naming conventions. The use of
API_STRfor path composition ensures consistency.src/framex/log.py (1)
18-19: LGTM!Deferring the
settingsimport to emission time is a good practice that reduces import-time dependencies and helps avoid potential circular import issues.tests/mock.py (1)
68-82: LGTM! Proper validation implemented.The previous review concern about bare
assertstatements has been addressed. The code now uses explicit validation with a clearValueError, ensuring the checks remain active even when Python runs with optimization flags.src/framex/plugins/proxy/config.py (2)
19-19: LGTM!The new
proxy_functionsfield provides a clean way to configure proxy function mappings per URL.
32-37: LGTM!The validator ensures configuration consistency by verifying that every
proxy_functionsURL has a corresponding entry inproxy_urls. This prevents misconfiguration at startup.src/framex/driver/ingress.py (1)
70-78: LGTM!Deferring
auth_keysresolution to registration time is a good practice. The local logger import and debug logging provide better visibility into authentication requirements for each route.src/framex/plugin/on.py (3)
101-106: LGTM!The early return when proxy is disabled is straightforward and clean. Note that the previous review concern about
proxy_onlyparameter handling is no longer applicable since that parameter has been removed from the decorator signature.
115-119: LGTM!Good validation: enforcing keyword-only arguments prevents confusion and makes proxy function calls explicit and readable.
128-134: LGTM!The proxy invocation flow correctly encodes the function name and arguments, calls the proxy, and decodes the result.
src/framex/config.py (1)
63-75: LGTM! Security-by-default approach.The renamed validator now performs both normalization and validation:
- Automatically adds
PROXY_FUNC_HTTP_PATHtoauth_urls, ensuring the proxy endpoint is always protected.- Generates a random authentication key if none is provided, logging a warning with the key for visibility.
This security-by-default approach is appropriate, though note that it introduces side effects (mutation) during validation, which is unconventional but pragmatic here.
tests/test_plugins.py (5)
8-9: LGTM: Import added for proxy testing.The
on_proxyimport is correctly placed and aligns with the new proxy function system being tested.
153-162: LGTM: Test models are well-structured.The
SubModelandExchangeModelclasses provide good test coverage for complex nested Pydantic model serialization through the proxy system.
169-171: LGTM: Remote-only test function correctly structured.The
RuntimeErrorand unused parameters are intentional—this function validates that remote proxy calls bypass local execution. The static analysis warnings (ARG001, TRY003) are false positives in this test context.
174-183: LGTM: Comprehensive local proxy test.The test properly validates local execution, including nested Pydantic model serialization and deserialization through the proxy system.
186-195: LGTM: Remote proxy test validates expected behavior.The test correctly validates the remote proxy execution path, checking that the function name and data are properly encoded and returned in the expected response format.
src/framex/plugin/__init__.py (4)
79-86: LGTM: Flexible API parameter handling.Accepting both
strandPluginApiimproves the API's flexibility for programmatic use while maintaining backward compatibility. The resolution logic correctly handles both cases.
89-102: LGTM: Robust proxy fallback mechanism.The automatic proxy creation for missing HTTP endpoints (paths starting with "/") provides graceful degradation. The warning log helps with debugging, and the error message for non-proxy cases is clear.
103-104: LGTM: Correct proxy detection.The
use_proxyflag is properly set based on the API's call type.
120-132: LGTM: Comprehensive proxy response validation.The response handling properly validates proxy responses:
- Non-dict results pass through directly
- Dict responses require
statusfield and validation againstlegal_proxy_code- Appropriate logging for errors and edge cases
src/framex/plugins/proxy/__init__.py (7)
12-12: LGTM: Necessary imports for proxy function system.The new imports (
PROXY_FUNC_HTTP_PATH,ProxyFunc,ProxyFuncHttpBody,cache_decode,cache_encode) are all utilized in the proxy function registration and invocation logic.Also applies to: 19-20
38-38: LGTM: Instance fields for proxy function management.The
proxy_func_mapregistry andinit_proxy_func_routeflag are appropriate for tracking registered proxy functions and route initialization state.Also applies to: 40-40
66-69: LGTM: Proper async context manager usage.The HTTP client is correctly managed with
async with, ensuring proper resource cleanup.
142-163: LGTM: Proxy function route registration is well-structured.The method correctly registers the HTTP endpoint for proxy function calls using the standardized path constant and appropriate parameters.
165-179: LGTM: Remote proxy function parsing is correctly implemented.The method properly creates dynamic proxy methods for remote functions, uses
ProxyFuncHttpBodyfor HTTP payloads, and registers them with theis_remote=Trueflag.
192-199: LGTM: Stream handling with proper cleanup.The streaming logic correctly uses
async withfor the stream context and ensures the client is closed after the generator completes. The pattern is valid for streaming responses.
202-208: LGTM: Robust non-streaming response handling.The method correctly uses an async context manager and gracefully handles both JSON and text responses with appropriate exception handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/framex/utils.py (1)
63-64: XOR obfuscation is not cryptographically secure.The
xor_cryptfunction uses a fixed key for XOR-based transformation, which provides obfuscation rather than cryptographic security. If the encoded data includes sensitive information or is transmitted over untrusted channels, consider using proper encryption (e.g.,cryptographylibrary with AES-GCM).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/framex/plugin/on.pysrc/framex/plugins/proxy/__init__.pysrc/framex/utils.pytests/test_plugins.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_plugins.py (1)
src/framex/plugin/on.py (1)
on_proxy(101-163)
src/framex/plugins/proxy/__init__.py (5)
src/framex/plugin/on.py (1)
on_request(63-98)src/framex/plugins/proxy/config.py (1)
ProxyPluginConfig(9-37)src/framex/plugins/proxy/model.py (2)
ProxyFunc(7-9)ProxyFuncHttpBody(12-14)src/framex/utils.py (2)
cache_decode(93-132)cache_encode(67-90)src/framex/adapter/base.py (2)
get_handle(53-53)call_func(29-39)
🪛 Ruff (0.14.10)
src/framex/utils.py
99-99: Do not catch blind exception: Exception
(BLE001)
105-105: Do not catch blind exception: Exception
(BLE001)
123-123: Do not catch blind exception: Exception
(BLE001)
tests/test_plugins.py
171-171: Avoid specifying long messages outside the exception class
(TRY003)
src/framex/plugins/proxy/__init__.py
292-292: Avoid specifying long messages outside the exception class
(TRY003)
src/framex/plugin/on.py
114-114: Do not catch blind exception: Exception
(BLE001)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (16)
src/framex/plugin/on.py (3)
101-110: Clean setup and lazy import pattern.The decorator initialization is well-structured. The lazy import of settings and early return when proxy is disabled are appropriate design choices.
111-125: Resilient callable wrapper with intentional fallback.The
safe_callablewrapper appropriately handles various function wrapping scenarios (classmethod, staticmethod, wrapped functions) with a fallback mechanism. The broad exception handling on line 114 is intentional for this fallback pattern, so the static analysis warning can be safely ignored.
127-161: Well-structured proxy invocation pattern.The wrapper correctly enforces keyword-only arguments (line 132), registers the function with the proxy deployment on first invocation, and routes subsequent calls through the proxy system with appropriate encoding. The registration and invocation flow integrates cleanly with the proxy infrastructure.
Note: A past review comment already identified a potential race condition with the
is_registeredflag (lines 134-146) and provided a detailed fix usingasyncio.Lock. Consider addressing that if concurrent registration is a concern.tests/test_plugins.py (4)
153-161: Test models appropriately structured.The
SubModelandExchangeModeldefinitions effectively test nested Pydantic model serialization and deserialization through the proxy system. The varying field types validate the encoding utilities handle different data structures correctly.
164-171: Proxy function test fixtures appropriately designed.The
local_exchange_key_valueandremote_exchange_key_valuefunctions correctly model local vs. remote execution patterns. TheRuntimeErrorin the remote function (line 171) is intentional—it validates that the function is invoked through the proxy system rather than directly. The static analysis warning about unused arguments (line 170) can be safely ignored.
174-184: Local proxy call test thoroughly validates nested structures.The test correctly validates that the local proxy function preserves data integrity through the proxy system, including nested Pydantic models. The assertions (lines 178-184) appropriately verify both primitive types and nested model attributes.
187-196: Remote proxy call test validates routing and encoding.The test appropriately validates that the remote proxy invocation correctly routes the function call, preserving the function name and argument data through the encoding/decoding pipeline. The assertions verify both the routing metadata and the transmitted data integrity.
src/framex/utils.py (2)
67-90: Encoding pipeline appropriately structures dynamic objects.The
cache_encodefunction correctly handles various Python types, including nested structures and Pydantic models. The exclusion of private attributes (line 70) and the multi-layer encoding pipeline (JSON → compression → XOR → base64) provide reasonable size reduction and obfuscation.The
dynamic_objstructure (lines 71-76) stores module and class information for later reconstruction—ensure that the decoding side validates this metadata against an allowlist if the data source is untrusted.
93-107: Decoding pipeline correctly reverses the encoding chain.The multi-stage decoding loop (lines 95-106) appropriately handles the layered encoding (base64 → XOR → decompression → JSON). The broad exception handling on lines 99 and 105 is intentional to gracefully handle partially-encoded or plain data, making the static analysis warnings false positives in this context.
src/framex/plugins/proxy/__init__.py (7)
38-40: Proxy function infrastructure initialized.The new fields
proxy_func_mapandinit_proxy_func_routeappropriately support the proxy function registration and routing system.
52-57: Proxy functions registration logic correctly structured.The conditional check (line 52) and iteration properly handle the presence or absence of proxy functions, with appropriate debug logging. This addresses the for-else logic issue from previous review comments.
65-69: HTTP client management follows best practices.The use of
async with httpx.AsyncClient()context manager (lines 66-69) ensures proper resource cleanup for the OpenAPI docs fetch operation.
195-211: HTTP client lifecycle appropriately managed.The
fetch_responsemethod correctly handles HTTP client lifecycle for both streaming (lines 195-202) and non-streaming (lines 205-211) scenarios. The streaming case creates a client that's explicitly closed after the generator completes, while the non-streaming case uses a context manager for automatic cleanup.
142-182: Proxy function routing infrastructure well-structured.The proxy function routing implementation cleanly separates concerns:
register_proxy_func_route(lines 142-163) registers the HTTP endpoint_proxy_func_route(lines 165-166) provides a simple forwarding handler_parse_proxy_function(lines 168-182) creates remote proxy function wrappersThe integration with the adapter's route registration system is appropriate.
266-278: Proxy function registration with duplicate detection.The method appropriately checks for duplicate registrations (lines 272-274) and logs function registration. The duplicate check prevents re-registering already-known functions in the proxy_func_map.
Note: A past review comment identified a potential race condition with the
init_proxy_func_routeflag (lines 269-271) and provided a detailed fix usingasyncio.Lock. Consider addressing that if concurrent registration is a concern.
280-292: Proxy function invocation routing implemented correctly.The method appropriately differentiates between remote (lines 284-285) and local (lines 286-287) proxy function invocations, with clear logging (line 289) and error handling for unregistered functions (line 292).
Note: This method uses
cache_decode(lines 281-282) to deserialize the function name and arguments. As flagged in thesrc/framex/utils.pyreview,cache_decodehas security implications when processing data from untrusted sources due to dynamic imports. Ensure appropriate authentication and validation are in place for the proxy endpoints.
| def restore_models(item: Any) -> Any: | ||
| if isinstance(item, list): | ||
| return [restore_models(i) for i in item] | ||
|
|
||
| if isinstance(item, dict): | ||
| if item.get("__type__") == "dynamic_obj": | ||
| try: | ||
| module = importlib.import_module(item["__module__"]) | ||
| cls = getattr(module, item["__class__"]) | ||
|
|
||
| cleaned_data = {k: restore_models(v) for k, v in item["data"].items()} | ||
|
|
||
| if hasattr(cls, "model_validate"): | ||
| return cls.model_validate(cleaned_data) | ||
| return cls(**cleaned_data) | ||
| except Exception: | ||
| from types import SimpleNamespace | ||
|
|
||
| return SimpleNamespace(**{k: restore_models(v) for k, v in item["data"].items()}) | ||
|
|
||
| return {k: restore_models(v) for k, v in item.items()} | ||
|
|
||
| return item | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the broader context of the restore_models function
cat -n src/framex/utils.py | head -150Repository: touale/FrameX-kit
Length of output: 5311
🏁 Script executed:
# Find where restore_models and cache_decode are used
rg -n "restore_models|cache_decode" --type pyRepository: touale/FrameX-kit
Length of output: 1302
🏁 Script executed:
# Check the on_proxy decorator and how the proxy system works
rg -n "on_proxy|@proxy" --type py -A 5Repository: touale/FrameX-kit
Length of output: 2313
🏁 Script executed:
# Look at the on_proxy decorator implementation
sed -n '101,200p' src/framex/plugin/on.pyRepository: touale/FrameX-kit
Length of output: 3633
🏁 Script executed:
# Check the proxy plugin implementation around cache_decode usage
sed -n '270,290p' src/framex/plugins/proxy/__init__.pyRepository: touale/FrameX-kit
Length of output: 1130
🏁 Script executed:
# Check how proxy function results are handled - are they encoded back?
rg -n "return.*cache_encode|cache_encode.*return" --type py -B 2 -A 2Repository: touale/FrameX-kit
Length of output: 496
🏁 Script executed:
# Check if there are any tests that show how proxy functions return data
cat tests/test_plugins.py | head -250Repository: touale/FrameX-kit
Length of output: 7630
🏁 Script executed:
# Let me check what the xor_crypt function actually does and if it's used for security
sed -n '63,91p' src/framex/utils.pyRepository: touale/FrameX-kit
Length of output: 1205
Critical: Arbitrary code execution risk via dynamic imports on decryptable, untrusted data.
Lines 115-116 use importlib.import_module and getattr to dynamically reconstruct objects based on __module__ and __class__ fields from decoded data. The encoding uses xor_crypt with a hardcoded key visible in source code—this is obfuscation, not encryption. An attacker with access to the codebase can:
- Craft malicious JSON with arbitrary
__module__and__class__values - Encrypt it using the known hardcoded XOR key
- Inject the payload as a proxy function result or other data path
- Trigger
importlib.import_moduleto load any installed module - Use
getattrto retrieve any class and instantiate it with attacker-controlled data, executing code via__init__
Required mitigation:
- Replace XOR encryption with authenticated encryption (e.g., using secrets module with HMAC-SHA256)
- Implement a strict allowlist of permitted modules and classes before
importlib.import_module - Validate decoded
dynamic_objentries against the allowlist and reject unrecognized entries - Consider using JSON-only serialization for untrusted data paths without dynamic object restoration
🧰 Tools
🪛 Ruff (0.14.10)
123-123: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In src/framex/utils.py around lines 108 to 131, restore_models currently
performs dynamic import/module/class lookup using untrusted "__module__" and
"__class__" fields and relies on XOR obfuscation with a hardcoded key; replace
this with a safe flow: remove or disable automatic import/getattr for untrusted
data and enforce a strict allowlist mapping of permitted module+class names to
constructor callables, validate the decoded "__type__" entries against that
allowlist and reject or return a safe serialized fallback for anything not
explicitly allowed, and remove the hardcoded XOR approach in favor of
authenticated integrity checks (e.g., HMAC or AEAD) so only verified payloads
are processed; ensure any allowed class construction uses validated,
type-checked cleaned_data and avoid calling arbitrary __init__ on untrusted
input.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/test_utils.py (1)
59-64: Remove duplicate imports.Lines 59-63 duplicate imports already declared at lines 1-5 (
json,Any,pytest,BaseModel). Remove these duplicate lines to improve code cleanliness.🔎 Proposed fix
- - -from datetime import datetime -from typing import Any - -import pytest -from pydantic import BaseModel - - class SubModel(BaseModel): id: int name: strsrc/framex/plugin/on.py (1)
134-146: Consider adding synchronization for registration.The
is_registeredflag is checked and set without synchronization. If multiple coroutines invoke the same proxy function concurrently before registration completes, multiple registration attempts could occur.While duplicate registrations may be idempotent, consider using an
asyncio.Lockto ensure only one coroutine performs the registration.🔎 Proposed fix using asyncio.Lock
+import asyncio + def on_proxy() -> Callable: def decorator(func: Callable) -> Callable: from framex.config import settings if not settings.server.enable_proxy: return func is_registered = False full_func_name = f"{func.__module__}.{func.__name__}" + registration_lock = asyncio.Lock() # ... safe_callable definition ... @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.") - if not is_registered: - api_reg = PluginApi( - deployment_name=PROXY_PLUGIN_NAME, - call_type=ApiType.PROXY, - func_name="register_proxy_function", - ) - await call_plugin_api( - api_reg, - None, - func_name=full_func_name, - func_callable=safe_callable, - ) - is_registered = True + async with registration_lock: + if not is_registered: # Double-check after acquiring lock + api_reg = PluginApi( + deployment_name=PROXY_PLUGIN_NAME, + call_type=ApiType.PROXY, + func_name="register_proxy_function", + ) + await call_plugin_api( + api_reg, + None, + func_name=full_func_name, + func_callable=safe_callable, + ) + is_registered = TrueBased on previous review feedback.
tests/test_plugins.py (1)
154-163: Consider extracting shared test models.
SubModelandExchangeModelare duplicated intests/test_utils.py(lines 66-74). Consider extracting these totests/conftest.pyas shared fixtures to reduce duplication.
📜 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 (6)
src/framex/__init__.pysrc/framex/config.pysrc/framex/plugin/on.pytests/test_cli.pytests/test_plugins.pytests/test_utils.py
💤 Files with no reviewable changes (1)
- tests/test_cli.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_utils.py (1)
src/framex/utils.py (4)
StreamEnventType(46-50)cache_decode(93-132)cache_encode(67-90)make_stream_event(53-60)
src/framex/plugin/on.py (3)
src/framex/plugin/model.py (3)
ApiType(22-26)PluginApi(29-37)PluginDeployment(41-43)src/framex/utils.py (3)
cache_decode(93-132)cache_encode(67-90)extract_method_params(36-43)src/framex/plugin/__init__.py (1)
call_plugin_api(78-133)
tests/test_plugins.py (2)
src/framex/plugin/on.py (4)
on_proxy(101-163)wrapper(72-96)wrapper(128-159)wrapper(167-224)tests/test_utils.py (2)
SubModel(66-68)ExchangeModel(71-75)
🪛 Ruff (0.14.10)
src/framex/plugin/on.py
114-114: Do not catch blind exception: Exception
(BLE001)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_plugins.py
168-168: Avoid specifying long messages outside the exception class
(TRY003)
181-181: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
src/framex/__init__.py (1)
101-101: LGTM! CPU auto-detection logic is correct.The conditional logic correctly passes
Noneto Ray whennum_cpus <= 0, enabling auto-detection. This aligns well with theServerConfig.num_cpusdefault change to-1insrc/framex/config.py.tests/test_utils.py (1)
78-127: LGTM! Comprehensive test coverage for encoding/decoding utilities.The test suite thoroughly covers:
- Basic data types and nested structures
- Datetime serialization
- Nested Pydantic models with proper reconstruction
- Fallback behavior when class definitions are unavailable
src/framex/config.py (2)
47-47: LGTM! Default change enables CPU auto-detection.Changing the default from
8to-1enables Ray's automatic CPU detection by default, which is more flexible for different deployment environments.
64-65: LGTM! Automatic proxy path protection is sensible.Automatically ensuring
PROXY_FUNC_HTTP_PATHis inauth_urlsprovides defense-in-depth for the proxy function feature.src/framex/plugin/on.py (2)
111-125: LGTM! Safe callable unwrapping logic is sound.The
safe_callablehelper correctly unwraps various decorator patterns (functools.wraps, classmethod, staticmethod) and handles both sync and async functions. The broad exception catch on line 114 is intentional as a fallback mechanism to ensure the raw function is accessible for proxy registration.Note: The static analysis warning about catching blind
Exceptionis a false positive in this context.
131-132: Keyword-only enforcement is correct.The check ensures proxy functions only accept keyword arguments, which is necessary for proper serialization with
cache_encode. The error message is clear and contextual.tests/test_plugins.py (2)
173-181: LGTM! Test function design is clear.The decorator stacking correctly exercises both local and remote proxy behavior. The
supply_exceptionwrapper simulates the remote execution path where the local function body is not executed.
184-206: LGTM! Comprehensive proxy function tests.Both test cases thoroughly validate:
- Local execution with nested model serialization/deserialization
- Remote proxy routing with proper data encoding
- Nested Pydantic model reconstruction
| if not self.general_auth_keys: # pragma: no cover | ||
| from framex.log import logger | ||
|
|
||
| key = str(uuid4()) | ||
| logger.warning(f"No general_auth_keys set, generate a random key: {key}") | ||
| self.general_auth_keys = [key] |
There was a problem hiding this comment.
Security concern: Auto-generated auth key is logged and may be weak.
Auto-generating a random authentication key and logging it has security implications:
- The key is exposed in logs, which may be collected/stored insecurely
- Users may not notice the warning and leave the weak default in place
- The auto-generation masks a configuration error
Consider either:
- Requiring explicit configuration and raising a clear error when
general_auth_keysis empty - Generating a strong key without logging it and providing it through a secure channel (e.g., startup output to stdout only, not logs)
🔎 Alternative approach: Require explicit configuration
if not self.general_auth_keys: # pragma: no cover
from framex.log import logger
- key = str(uuid4())
- logger.warning(f"No general_auth_keys set, generate a random key: {key}")
- self.general_auth_keys = [key]
+ raise ValueError(
+ "general_auth_keys must be explicitly configured for authentication. "
+ "Please set at least one auth key in your configuration."
+ )🤖 Prompt for AI Agents
In src/framex/config.py around lines 66-71, the code auto-generates an auth key
and writes it to the logger which exposes secrets and masks a config error;
change behavior to require explicit configuration by raising a clear exception
when general_auth_keys is empty (include a message that instructs how to provide
keys via env/config) OR if you must auto-generate for convenience, generate a
cryptographically strong key (secure random, sufficient length) and do NOT log
it — instead print a single non-persisted startup notice to stdout informing the
operator that a key was generated and where to find it (or write it to a secure
file path), and set self.general_auth_keys to that value; remove logger.warning
that prints the secret.
| def supply_execption(func): | ||
| @wraps(func) | ||
| async def wrapper(*_: Any, **__: Any) -> None: | ||
| raise RuntimeError("I am def supply_execption(func): exception") | ||
|
|
||
| return wrapper |
There was a problem hiding this comment.
Typo in function name: supply_execption should be supply_exception.
The function name has a typo that should be corrected for clarity.
🔎 Proposed fix
-def supply_execption(func):
+def supply_exception(func):
@wraps(func)
async def wrapper(*_: Any, **__: Any) -> None:
raise RuntimeError("I am def supply_execption(func): exception")
return wrapperAlso update line 179:
@on_proxy()
-@supply_execption
+@supply_exception
async def remote_exchange_key_value(a_str: str, b_int: int, c_model: ExchangeModel) -> Any: # noqa: ARG001And update the error message on line 168:
- raise RuntimeError("I am def supply_execption(func): exception")
+ raise RuntimeError("I am def supply_exception(func): exception")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
168-168: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In tests/test_plugins.py around lines 165 to 170, the helper function is
misspelled as supply_execption; rename it to supply_exception, update its
internal RuntimeError message to reflect the corrected name (line ~168), and
update any references/calls to this function (e.g., the use at line ~179) to use
supply_exception so names and messages are consistent.
1 participant
Summary by CodeRabbit
New Features
Behavior Changes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.