Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduce an ApiResolver abstraction and ContextVar-based resolver/remote-API context; bind per-plugin api_registry/resolver for request handlers; change call_plugin_api to use resolver/context; reinitialize adapter and Sentry inside Ray worker processes; add coverage startup support for subprocesses and merge coverage shards in tests; add resolver- and Ray-focused tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BasePlugin
participant ApiResolver
participant Adapter
participant RemoteAPI
Client->>BasePlugin: HTTP request -> handler
BasePlugin->>ApiResolver: set_current_api_resolver(self.api_resolver)
BasePlugin->>ApiResolver: set_current_remote_apis(self.api_registry)
BasePlugin->>ApiResolver: call_plugin_api(api_name, **kwargs)
ApiResolver->>Adapter: resolve -> PluginApi
Adapter->>RemoteAPI: invoke (function or HTTP)
RemoteAPI-->>Adapter: response
Adapter-->>BasePlugin: return result
BasePlugin->>ApiResolver: reset_current_remote_apis()
BasePlugin->>ApiResolver: reset_current_api_resolver()
BasePlugin-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
touale
force-pushed
the
fix-plugin-api-not-found
branch
from
8e5bbea to
940761f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
tests/test_plugin.py (1)
17-17: Move import to the top of the file.The
import inspectstatement is placed after the first test function, which deviates from Python convention (PEP 8) of grouping all imports at the top of the module.📦 Suggested fix
Move
import inspectto line 1 with the other imports:+import inspect + import framex from framex.consts import VERSIONAnd remove line 17.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_plugin.py` at line 17, Move the "import inspect" statement from its current location after the first test function up to the top of the module with the other imports and remove the duplicate import where it currently appears; locate the stray "import inspect" and relocate it to the module import block so all imports are grouped at the top following PEP8.src/framex/plugin/__init__.py (3)
216-227: Consider exportingApiResolverand context helpers in__all__.The new public utilities (
ApiResolver,set_current_api_resolver,reset_current_api_resolver,get_current_remote_apis,set_current_remote_apis,reset_current_remote_apis) are imported by tests but not listed in__all__. If these are intended to be part of the public API (e.g., for advanced plugin developers), they should be exported.📦 Suggested addition
__all__ = [ + "ApiResolver", "ApiType", "BasePlugin", "PluginMetadata", + "get_current_api_resolver", + "get_current_remote_apis", "load_builtin_plugins", "load_plugins", "on_proxy", "on_register", "on_request", "register_proxy_func", "remote", + "reset_current_api_resolver", + "reset_current_remote_apis", + "set_current_api_resolver", + "set_current_remote_apis", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/plugin/__init__.py` around lines 216 - 227, The __all__ export list in framex.plugin.__init__ is missing the new public utilities used by tests; add "ApiResolver", "set_current_api_resolver", "reset_current_api_resolver", "get_current_remote_apis", "set_current_remote_apis", and "reset_current_remote_apis" to the __all__ list so these symbols are exported from the package (they are the identifiers imported elsewhere and implemented alongside ApiType/BasePlugin/PluginMetadata).
34-40: Consider logging or raising for unexpected types in_coerce_plugin_api.The method silently returns
Nonefor types that are neitherPluginApi,dict, norNone. This could mask bugs if an unexpected type is passed.🔍 Suggested enhancement
`@staticmethod` def _coerce_plugin_api(api: PluginApi | dict[str, Any] | None) -> PluginApi | None: if api is None or isinstance(api, PluginApi): return api if isinstance(api, dict): return PluginApi.model_validate(api) + logger.warning(f"Unexpected type for plugin API coercion: {type(api)}") return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/plugin/__init__.py` around lines 34 - 40, The helper _coerce_plugin_api currently returns None for unexpected types which can hide bugs; update _coerce_plugin_api to either raise a TypeError (including the actual type received) or log an error before returning None so callers get immediate feedback — locate the _coerce_plugin_api staticmethod and replace the final `return None` path with a TypeError that mentions the received value/type (or call your module logger with a clear message) while keeping the existing branches that accept PluginApi, dict, and None and continuing to use PluginApi.model_validate for dict inputs.
104-104:ApiTypeused before import statement.
ApiType.FUNCandApiType.HTTPare referenced here, butApiTypeis imported at line 213 (bottom of file). While this works at runtime because function bodies execute after module loading completes, it causes:
- IDE/type-checker confusion
- Potential issues if this function is called during module initialization
- Reduced code readability
📦 Suggested fix
Add
ApiTypeto the existing import fromframex.plugin.modelat line 13:-from framex.plugin.model import Plugin, PluginApi +from framex.plugin.model import ApiType, Plugin, PluginApiThen remove
ApiTypefrom the bottom import at line 213:-from .model import ApiType, PluginMetadata +from .model import PluginMetadata🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/plugin/__init__.py` at line 104, The code references ApiType (used in the all_apis assignment with _manager) before ApiType is imported later in the file; update the top import from framex.plugin.model to include ApiType (so ApiType is available when all_apis is defined), and remove the duplicate/bottom import of ApiType at the end of the file to avoid redundancy and IDE/type-checker confusion; ensure you modify the import grouping that currently brings in models (the import that already brings in other symbols from framex.plugin.model) and then delete the later ApiType import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest.ini`:
- Around line 21-22: COVERAGE_PROCESS_START in pytest.ini will not trigger
coverage.process_startup() in spawned subprocesses unless sitecustomize.py is
importable; update pytest.ini so the env block also prepends
tools/coverage_support to subprocess PYTHONPATH (e.g. set
PYTHONPATH=tools/coverage_support:$PYTHONPATH alongside COVERAGE_PROCESS_START)
so sitecustomize.py can be imported by child interpreters, or alternatively
remove COVERAGE_PROCESS_START from pytest.ini and handle coverage startup
per-test as done in tests/test_use_ray_integration.py; reference:
COVERAGE_PROCESS_START, sitecustomize.py, tools/coverage_support,
tests/test_use_ray_integration.py.
In `@src/framex/__init__.py`:
- Around line 69-75: The driver process never flips the Ray mode flag, so calls
like framex.adapter.get_adapter() and BasePlugin.__init__ still see local mode;
mirror the worker-side change by setting settings.server.use_ray = True in the
driver startup path when run(use_ray=True) is invoked (the same place that
currently prepares workers), and also reset adapter state
(adapter_module._adapter = None) there so the driver and workers have consistent
mode; apply the same update to the other startup site noted (the other
invocation that prepares Ray) so both entry paths set settings.server.use_ray
and clear adapter state.
In `@src/framex/plugin/base.py`:
- Around line 85-88: The helper _call_remote_api currently always uses the
global resolver via call_plugin_api and loses any instance-scoped resolver,
causing calls from on_start or other non-request paths to miss plugin-declared
APIs; modify _call_remote_api to detect and pass the plugin instance's
resolver/fallback (e.g., self.remote_apis or self._remote_api_resolver) into
call_plugin_api when present, falling back to the global resolver only if the
instance-scoped resolver is None, and then continue to return
self._post_call_remote_api_hook(res).
In `@tests/conftest.py`:
- Around line 75-78: The try/except around "from coverage import Coverage" is
currently catching all exceptions; change the except block to only catch
ImportError (e.g., except ImportError:) so that only a missing coverage package
is ignored and any other errors during import or subsequent setup are allowed to
surface; locate the import statement referencing the Coverage symbol and replace
the broad except Exception with an except ImportError handler (optionally
capturing the exception as e if you want to log it) so the merge/coverage logic
isn't silently skipped.
In `@tests/test_use_ray_integration.py`:
- Around line 99-107: The test uses subprocess.Popen(...,
start_new_session=True) to launch Ray but cleanup currently calls
proc.terminate()/proc.kill() which only targets the launcher process; change the
cleanup logic to kill the entire process group using
os.killpg(os.getpgid(proc.pid), signal.SIGTERM) (and fall back to SIGKILL if
needed) instead of os.kill(proc.pid, ...), ensuring you import os and signal and
handle OSError for already-exited processes; locate the teardown code that
references proc.terminate()/proc.kill() in tests/test_use_ray_integration.py and
replace it with os.killpg-based group termination for the proc launched by
subprocess.Popen.
In `@tools/coverage_support/sitecustomize.py`:
- Around line 4-8: The current suppress(Exception) around importing and calling
coverage.process_startup() hides non-ImportError issues (like
coverage.ConfigError); update the exception filter to only suppress ImportError
so missing coverage installs remain quiet but real coverage misconfigurations
surface. Locate the block that checks os.getenv("COVERAGE_PROCESS_START") and
replace suppress(Exception) with suppress(ImportError) around the import
coverage and coverage.process_startup() call.
---
Nitpick comments:
In `@src/framex/plugin/__init__.py`:
- Around line 216-227: The __all__ export list in framex.plugin.__init__ is
missing the new public utilities used by tests; add "ApiResolver",
"set_current_api_resolver", "reset_current_api_resolver",
"get_current_remote_apis", "set_current_remote_apis", and
"reset_current_remote_apis" to the __all__ list so these symbols are exported
from the package (they are the identifiers imported elsewhere and implemented
alongside ApiType/BasePlugin/PluginMetadata).
- Around line 34-40: The helper _coerce_plugin_api currently returns None for
unexpected types which can hide bugs; update _coerce_plugin_api to either raise
a TypeError (including the actual type received) or log an error before
returning None so callers get immediate feedback — locate the _coerce_plugin_api
staticmethod and replace the final `return None` path with a TypeError that
mentions the received value/type (or call your module logger with a clear
message) while keeping the existing branches that accept PluginApi, dict, and
None and continuing to use PluginApi.model_validate for dict inputs.
- Line 104: The code references ApiType (used in the all_apis assignment with
_manager) before ApiType is imported later in the file; update the top import
from framex.plugin.model to include ApiType (so ApiType is available when
all_apis is defined), and remove the duplicate/bottom import of ApiType at the
end of the file to avoid redundancy and IDE/type-checker confusion; ensure you
modify the import grouping that currently brings in models (the import that
already brings in other symbols from framex.plugin.model) and then delete the
later ApiType import.
In `@tests/test_plugin.py`:
- Line 17: Move the "import inspect" statement from its current location after
the first test function up to the top of the module with the other imports and
remove the duplicate import where it currently appears; locate the stray "import
inspect" and relocate it to the module import block so all imports are grouped
at the top following PEP8.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ebd580e-2656-4743-b55c-f3ce8869185b
📒 Files selected for processing (13)
.coveragercpytest.inisrc/framex/__init__.pysrc/framex/plugin/__init__.pysrc/framex/plugin/base.pytests/conftest.pytests/ray_plugins/__init__.pytests/ray_plugins/test_service_caller.pytests/test_exception.pytests/test_plugin.pytests/test_use_ray_integration.pytools/coverage_support/__init__.pytools/coverage_support/sitecustomize.py
| def _setup_ray_worker() -> None: # pragma: no cover | ||
| settings.server.use_ray = True | ||
|
|
||
| import framex.adapter as adapter_module | ||
|
|
||
| adapter_module._adapter = None | ||
| _setup_sentry() |
There was a problem hiding this comment.
Mirror the Ray-mode switch in the driver process.
_setup_ray_worker() fixes worker-side state, but run(use_ray=True) still leaves settings.server.use_ray unchanged in the parent process. Anything that reads the setting before workers start—like framex.adapter.get_adapter() and BasePlugin.__init__()—still sees local mode unless callers mutate settings themselves, which is why tests/test_use_ray_integration.py Lines 45 and 96 have to do that manually.
Suggested fix
num_cpus = num_cpus if num_cpus is not None else settings.server.num_cpus
use_ray = use_ray if use_ray is not None else settings.server.use_ray
+ settings.server.use_ray = use_ray
enable_proxy = enable_proxy if enable_proxy is not None else settings.server.enable_proxyAlso applies to: 158-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/__init__.py` around lines 69 - 75, The driver process never flips
the Ray mode flag, so calls like framex.adapter.get_adapter() and
BasePlugin.__init__ still see local mode; mirror the worker-side change by
setting settings.server.use_ray = True in the driver startup path when
run(use_ray=True) is invoked (the same place that currently prepares workers),
and also reset adapter state (adapter_module._adapter = None) there so the
driver and workers have consistent mode; apply the same update to the other
startup site noted (the other invocation that prepares Ray) so both entry paths
set settings.server.use_ray and clear adapter state.
| proc = subprocess.Popen( # noqa: S603 | ||
| [sys.executable, "-c", launch_code], | ||
| cwd=repo_root, | ||
| env=env, | ||
| stdout=log_file, | ||
| stderr=subprocess.STDOUT, | ||
| text=True, | ||
| start_new_session=True, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/test_use_ray_integration.py | head -130 | tail -40Repository: touale/FrameX-kit
Length of output: 1843
🏁 Script executed:
cat -n tests/test_use_ray_integration.pyRepository: touale/FrameX-kit
Length of output: 4973
🏁 Script executed:
python3 << 'EOF'
import subprocess
import sys
# Test 1: Check what Popen.terminate() actually does
# On POSIX, it calls os.kill(self.pid, signal.SIGTERM)
# NOT os.killpg() which would kill the process group
# Let's check the subprocess module source documentation
help(subprocess.Popen.terminate)
EOFRepository: touale/FrameX-kit
Length of output: 166
🏁 Script executed:
rg -i "killpg|process.group|session" tests/ src/ --type py -A 3 -B 3Repository: touale/FrameX-kit
Length of output: 1924
🏁 Script executed:
# Check if there are any other tests or examples using start_new_session=True
rg "start_new_session" --type py -A 5 -B 5Repository: touale/FrameX-kit
Length of output: 755
🏁 Script executed:
# Let's verify the actual behavior described in the review
# Check if the subprocess module's terminate() sends signal only to main process or whole group
python3 << 'EOF'
import os
import signal
# On POSIX, Popen.terminate() calls os.kill(self.pid, signal.SIGTERM)
# This sends signal to a single process, NOT the process group
# If we want to kill a process group created with start_new_session=True,
# we need os.killpg(os.getpgid(pid), signal.SIGTERM)
# OR os.killpg(pid, signal.SIGTERM) if pid is the group leader
print("Key facts about process groups with start_new_session=True:")
print("1. start_new_session=True creates a new process group on POSIX")
print("2. The subprocess becomes a session and process group leader")
print("3. proc.terminate() calls os.kill(pid, SIGTERM) - single process only")
print("4. proc.kill() calls os.kill(pid, SIGKILL) - single process only")
print("5. To kill the group: os.killpg(proc.pid, SIGTERM) works because proc.pid is the group leader")
print("6. Any child processes started by proc become part of the same group")
print("7. Only signaling the group leader won't kill the children unless it exits")
EOFRepository: touale/FrameX-kit
Length of output: 617
🏁 Script executed:
# Check launch_code more carefully - does it create child processes?
rg -i "ray\.|serve\." tests/test_use_ray_integration.py -A 2Repository: touale/FrameX-kit
Length of output: 271
Kill the entire Ray session group on cleanup, not just the launcher process.
start_new_session=True creates a new process group where the launcher becomes the group leader. When cleanup calls proc.terminate() or proc.kill(), it only signals that single process via os.kill(). If the launcher times out before its serve.shutdown()/ray.shutdown() completes, Ray's worker and dashboard child processes—which are in the same process group—survive cleanup and leak resources (ports, processes) that poison subsequent tests.
Use os.killpg() to terminate the entire process group:
Suggested fix
import json
import os
import socket
+import signal
import subprocess
import sys
import tempfile
@@
if proc.poll() is None:
- proc.terminate()
+ if os.name == "posix":
+ os.killpg(proc.pid, signal.SIGTERM)
+ else:
+ proc.terminate()
with suppress(subprocess.TimeoutExpired):
proc.wait(timeout=10)
if proc.poll() is None:
- proc.kill()
+ if os.name == "posix":
+ os.killpg(proc.pid, signal.SIGKILL)
+ else:
+ proc.kill()
proc.wait(timeout=10)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_use_ray_integration.py` around lines 99 - 107, The test uses
subprocess.Popen(..., start_new_session=True) to launch Ray but cleanup
currently calls proc.terminate()/proc.kill() which only targets the launcher
process; change the cleanup logic to kill the entire process group using
os.killpg(os.getpgid(proc.pid), signal.SIGTERM) (and fall back to SIGKILL if
needed) instead of os.kill(proc.pid, ...), ensuring you import os and signal and
handle OSError for already-exited processes; locate the teardown code that
references proc.terminate()/proc.kill() in tests/test_use_ray_integration.py and
replace it with os.killpg-based group termination for the proc launched by
subprocess.Popen.
| if os.getenv("COVERAGE_PROCESS_START"): | ||
| with suppress(Exception): | ||
| import coverage | ||
|
|
||
| coverage.process_startup() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tools/coverage_support/sitecustomize.pyRepository: touale/FrameX-kit
Length of output: 289
🏁 Script executed:
wc -l tools/coverage_support/sitecustomize.pyRepository: touale/FrameX-kit
Length of output: 102
🏁 Script executed:
# Search for other uses of suppress(Exception) or similar patterns in the codebase
rg "suppress\(Exception\)" -A 3 -B 1Repository: touale/FrameX-kit
Length of output: 390
🏁 Script executed:
# Check if there are any comments or documentation about why broad exception suppression was chosen
rg "COVERAGE_PROCESS_START" -B 5 -A 10Repository: touale/FrameX-kit
Length of output: 1616
🏁 Script executed:
# Look at git history to understand context if available
git log --oneline tools/coverage_support/sitecustomize.py 2>/dev/null | head -20Repository: touale/FrameX-kit
Length of output: 121
🏁 Script executed:
# Look for documentation or comments about coverage.process_startup behavior
rg "process_startup" -B 3 -A 3Repository: touale/FrameX-kit
Length of output: 310
🏁 Script executed:
# Check if there are any other sitecustomize files in related projects or docs
find . -name "sitecustomize.py" -o -name "*.md" | xargs grep -l "sitecustomize" 2>/dev/null | head -5Repository: touale/FrameX-kit
Length of output: 43
🏁 Script executed:
# Look at the commit that introduced this to understand the context
git show 940761f --stat 2>/dev/null | head -30Repository: touale/FrameX-kit
Length of output: 1709
🏁 Script executed:
# Search for any error handling patterns or discussion about coverage in issues/tests
rg "coverage" -i tools/coverage_support/ -A 2 -B 2Repository: touale/FrameX-kit
Length of output: 503
🌐 Web query:
coverage.py process_startup() exceptions documentation
💡 Result:
The coverage.py process_startup function is documented in the official Coverage.py API module documentation. It is used to start coverage measurement automatically at Python startup for subprocesses. Signature (Coverage.py 7.13.4+): coverage.process_startup(*, force=False, slug='default') It checks environment variables COVERAGE_PROCESS_START or COVERAGE_PROCESS_CONFIG. If set, it loads the specified config and starts a Coverage instance. Returns the Coverage instance or None if not started. No specific exceptions are documented as being raised directly by process_startup. Coverage.py raises subclasses of CoverageException in general, including: - ConfigError: problems with config files or values. - CoverageWarning: warnings (not exceptions). - DataError: errors with data files. - NoCode: no code found. - NoDataError: no data. - NoSource: source not found. - NotPython: not parsable Python. - PluginError: plugin issues. Coverage class methods (used internally) can raise these exceptions. Misuse, like calling in the main process run by 'coverage run', can lead to AssertionError in the collector (as seen in user reports), but this is not official behavior—process_startup is for sitecustomize.py or .pth files in subprocess contexts. Full details: https://coverage.readthedocs.io/en/latest/api_module.html and https://coverage.readthedocs.io/en/latest/subprocess.html
Citations:
- 1: https://coverage.readthedocs.io/en/latest/api_module.html
- 2: https://coverage.readthedocs.io/en/latest/subprocess.html
- 3: https://coverage.readthedocs.io/en/latest/api_exceptions.html
- 4: https://coverage.readthedocs.io/en/latest/api_coverage.html
Catch only ImportError to surface coverage misconfigurations.
The broad suppress(Exception) silently swallows ConfigError and other coverage-related exceptions, masking misconfiguration and causing undercounting without CI failure. Only ImportError (when coverage is not installed) should be suppressed.
Suggested fix
import os
-from contextlib import suppress
if os.getenv("COVERAGE_PROCESS_START"):
- with suppress(Exception):
- import coverage
-
- coverage.process_startup()
+ try:
+ import coverage
+ except ImportError:
+ pass
+ else:
+ coverage.process_startup()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/coverage_support/sitecustomize.py` around lines 4 - 8, The current
suppress(Exception) around importing and calling coverage.process_startup()
hides non-ImportError issues (like coverage.ConfigError); update the exception
filter to only suppress ImportError so missing coverage installs remain quiet
but real coverage misconfigurations surface. Locate the block that checks
os.getenv("COVERAGE_PROCESS_START") and replace suppress(Exception) with
suppress(ImportError) around the import coverage and coverage.process_startup()
call.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/framex/__init__.py (1)
69-75:⚠️ Potential issue | 🟠 MajorMirror Ray mode + adapter reset in the driver path as well.
This worker-only setup is good, but
run(use_ray=True)still doesn’t update driver state before plugin/deployment initialization. Please setsettings.server.use_rayand clearframex.adapter._adapterinrun()right after resolvinguse_ray, so driver and workers stay consistent.Suggested fix
@@ num_cpus = num_cpus if num_cpus is not None else settings.server.num_cpus use_ray = use_ray if use_ray is not None else settings.server.use_ray + settings.server.use_ray = use_ray + if use_ray: + import framex.adapter as adapter_module + adapter_module._adapter = None enable_proxy = enable_proxy if enable_proxy is not None else settings.server.enable_proxy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/__init__.py` around lines 69 - 75, The driver path currently doesn't mirror the worker-only Ray setup: after resolving the use_ray flag in run(), set settings.server.use_ray = True/False accordingly and clear framex.adapter._adapter (set to None) just like _setup_ray_worker() does, so driver and workers share the same Ray mode before plugin/deployment initialization; update the run() function to perform these two actions immediately after resolving use_ray.tests/conftest.py (1)
75-78:⚠️ Potential issue | 🟠 MajorNarrow the import exception to
ImportErroronly.Line 77 catches
Exception, which can hide real coverage/config failures and silently skip merge. Restrict this toImportErrorso non-import failures surface.Suggested fix
try: from coverage import Coverage - except Exception: + except ImportError: return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 75 - 78, The try/except around importing Coverage in tests/conftest.py currently catches all Exceptions which can hide real errors; change the broad except to except ImportError so only missing-imports are suppressed—locate the try: from coverage import Coverage block and replace "except Exception:" with "except ImportError:" ensuring other exceptions propagate.src/framex/plugin/base.py (1)
86-88:⚠️ Potential issue | 🟠 MajorKeep
_call_remote_api()instance-scoped outside request handlers.This still only works when the call happens under one of the wrapped
@on_requestmethods.on_start()at Line 31 is not wrapped, so worker-side calls can still fall back to the global resolver and raiseAPI ... is not foundeven when the plugin declared that dependency.Suggested fix
`@final` async def _call_remote_api(self, api_name: str, **kwargs: Any) -> Any: - res = await call_plugin_api(api_name, **kwargs) + resolver_token = set_current_api_resolver(self.api_resolver) + remote_token = set_current_remote_apis(self.remote_apis) + try: + res = await call_plugin_api(api_name, resolver=self.api_resolver, **kwargs) + finally: + reset_current_remote_apis(remote_token) + reset_current_api_resolver(resolver_token) return self._post_call_remote_api_hook(res)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/plugin/base.py` around lines 86 - 88, The current _call_remote_api calls the global call_plugin_api which relies on a global resolver and fails for worker-side calls (e.g., on_start); change _call_remote_api to use an instance-scoped API caller stored on the plugin instance (instead of the global call_plugin_api). Concretely, introduce or use an instance attribute (e.g., self._call_plugin_api or self.call_plugin_api) that is initialized during plugin construction/initialization and replace call_plugin_api(...) with self._call_plugin_api(...), keeping the existing post-call handling via _post_call_remote_api_hook(res); ensure the instance-scoped caller is available for on_start and other non-@on_request handlers.
🧹 Nitpick comments (1)
tests/test_plugin.py (1)
195-213: Assert the serialized API payload is rehydrated before dispatch.This test still passes if
call_plugin_api()skipsPluginApi.model_validate()and forwards the rawdictintocall_func(). Add an assertion on the first adapter argument being aPluginApi(or matchingapi) so the Ray serialization path is actually covered.Suggested test strengthening
mock_get_api.assert_not_called() assert result == "interval_dict_result" + dispatched_api = mock_adapter.return_value.call_func.call_args[0][0] + assert isinstance(dispatched_api, PluginApi) + assert dispatched_api.api == api.api + assert dispatched_api.deployment_name == api.deployment_name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_plugin.py` around lines 195 - 213, The test test_call_plugin_api_with_interval_apis_dict_payload can pass even if call_plugin_api forwards the raw dict; update the test to assert the adapter's call_func was invoked with a rehydrated PluginApi instance by checking the first positional argument passed to mock_adapter.return_value.call_func is an instance equal to (or has the same attributes as) the original api — i.e., after obtaining result, inspect mock_adapter.return_value.call_func.call_args or call_args_list and assert isinstance(first_arg, PluginApi) or first_arg.model_dump() == api.model_dump(); this ensures call_plugin_api invokes PluginApi.model_validate and doesn't forward the raw dict from set_current_remote_apis.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/framex/__init__.py`:
- Around line 69-75: The driver path currently doesn't mirror the worker-only
Ray setup: after resolving the use_ray flag in run(), set
settings.server.use_ray = True/False accordingly and clear
framex.adapter._adapter (set to None) just like _setup_ray_worker() does, so
driver and workers share the same Ray mode before plugin/deployment
initialization; update the run() function to perform these two actions
immediately after resolving use_ray.
In `@src/framex/plugin/base.py`:
- Around line 86-88: The current _call_remote_api calls the global
call_plugin_api which relies on a global resolver and fails for worker-side
calls (e.g., on_start); change _call_remote_api to use an instance-scoped API
caller stored on the plugin instance (instead of the global call_plugin_api).
Concretely, introduce or use an instance attribute (e.g., self._call_plugin_api
or self.call_plugin_api) that is initialized during plugin
construction/initialization and replace call_plugin_api(...) with
self._call_plugin_api(...), keeping the existing post-call handling via
_post_call_remote_api_hook(res); ensure the instance-scoped caller is available
for on_start and other non-@on_request handlers.
In `@tests/conftest.py`:
- Around line 75-78: The try/except around importing Coverage in
tests/conftest.py currently catches all Exceptions which can hide real errors;
change the broad except to except ImportError so only missing-imports are
suppressed—locate the try: from coverage import Coverage block and replace
"except Exception:" with "except ImportError:" ensuring other exceptions
propagate.
---
Nitpick comments:
In `@tests/test_plugin.py`:
- Around line 195-213: The test
test_call_plugin_api_with_interval_apis_dict_payload can pass even if
call_plugin_api forwards the raw dict; update the test to assert the adapter's
call_func was invoked with a rehydrated PluginApi instance by checking the first
positional argument passed to mock_adapter.return_value.call_func is an instance
equal to (or has the same attributes as) the original api — i.e., after
obtaining result, inspect mock_adapter.return_value.call_func.call_args or
call_args_list and assert isinstance(first_arg, PluginApi) or
first_arg.model_dump() == api.model_dump(); this ensures call_plugin_api invokes
PluginApi.model_validate and doesn't forward the raw dict from
set_current_remote_apis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d1fd49d-8ca4-4c07-b296-f4ea2e978e07
📒 Files selected for processing (13)
.coveragercpytest.inisrc/framex/__init__.pysrc/framex/plugin/__init__.pysrc/framex/plugin/base.pytests/conftest.pytests/ray_plugins/__init__.pytests/ray_plugins/test_service_caller.pytests/test_exception.pytests/test_plugin.pytests/test_use_ray_integration.pytools/coverage_support/__init__.pytools/coverage_support/sitecustomize.py
✅ Files skipped from review due to trivial changes (4)
- pytest.ini
- tools/coverage_support/sitecustomize.py
- .coveragerc
- tests/ray_plugins/test_service_caller.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_exception.py
- tests/test_use_ray_integration.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/framex/plugin/base.py (1)
85-87:⚠️ Potential issue | 🟠 MajorReapply the instance resolver in
_call_remote_api.Line 87 now relies entirely on ambient context, but the wrapper at Lines 36-83 only runs for
_on_requesthandlers. Calls fromon_start()or any other non-request path can still missself.remote_apis/self.api_resolverand regress toAPI ... is not foundin worker-local execution.Suggested fix
`@final` async def _call_remote_api(self, api_name: str, **kwargs: Any) -> Any: - res = await call_plugin_api(api_name, **kwargs) + resolver_token = set_current_api_resolver(self.api_resolver) + remote_token = set_current_remote_apis(self.remote_apis) + try: + res = await call_plugin_api(api_name, resolver=self.api_resolver, **kwargs) + finally: + reset_current_remote_apis(remote_token) + reset_current_api_resolver(resolver_token) return self._post_call_remote_api_hook(res)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/plugin/base.py` around lines 85 - 87, The _call_remote_api currently calls call_plugin_api without restoring the instance resolver context, so calls from non-request paths can miss self.remote_apis/self.api_resolver and fail; update _call_remote_api to reapply the same resolver wiring used by the _on_request wrapper: if self.api_resolver or self.remote_apis exists, pass that resolver/context into call_plugin_api (e.g. via api_resolver/remote_apis kwargs or by invoking the same resolver restoration helper used by _on_request) so remote API lookups use the instance-specific resolver.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framex/plugin/__init__.py`:
- Around line 90-93: The current early return when api_name is a PluginApi
bypasses the remote-apis guard; after calling get_current_remote_apis() you must
validate pre-resolved PluginApi inputs against the same
required_remote_apis/current_remote_apis rules as string names. Modify the
branch that handles isinstance(api_name, PluginApi) (and uses api_name.call_type
/ ApiType.PROXY) to first consult get_current_remote_apis() and
required_remote_apis and only allow the PluginApi if the remote_apis guard
permits it (otherwise raise or reject); ensure the same validation logic used
for string api_name paths is reused for PluginApi instances.
- Around line 94-98: active_resolver may be None because
get_current_api_resolver() and get_default_api_resolver() return optionals;
ensure active_resolver is non-None before using it by providing a final fallback
resolver instance (e.g., construct or call a known default ApiResolver) when
resolver or the two getters return None; modify the assignment of
active_resolver and subsequent uses in this block (references: active_resolver,
resolver, get_current_api_resolver, get_default_api_resolver) so that
coerce_plugin_api and resolve are only called on a guaranteed resolver object,
and keep existing branches using current_remote_apis and api_name unchanged.
---
Duplicate comments:
In `@src/framex/plugin/base.py`:
- Around line 85-87: The _call_remote_api currently calls call_plugin_api
without restoring the instance resolver context, so calls from non-request paths
can miss self.remote_apis/self.api_resolver and fail; update _call_remote_api to
reapply the same resolver wiring used by the _on_request wrapper: if
self.api_resolver or self.remote_apis exists, pass that resolver/context into
call_plugin_api (e.g. via api_resolver/remote_apis kwargs or by invoking the
same resolver restoration helper used by _on_request) so remote API lookups use
the instance-specific resolver.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 844b26f1-115d-4345-b53c-6a1ae281490b
📒 Files selected for processing (5)
src/framex/plugin/__init__.pysrc/framex/plugin/base.pysrc/framex/plugin/manage.pysrc/framex/plugin/resolver.pytests/test_plugin.py
| current_remote_apis = get_current_remote_apis() | ||
| if isinstance(api_name, PluginApi): | ||
| api: PluginApi | None = api_name | ||
| elif isinstance(api_name, str): | ||
| api = interval_apis.get(api_name) if interval_apis else _manager.get_api(api_name) | ||
| use_proxy = False | ||
| if not api: | ||
| if isinstance(api_name, str) and api_name.startswith("/") and settings.server.enable_proxy: | ||
| return api_name, api_name.call_type == ApiType.PROXY | ||
|
|
There was a problem hiding this comment.
Apply the remote_apis guard to pre-resolved PluginApi inputs too.
Lines 91-93 return immediately for PluginApi arguments, so once current_remote_apis is set a caller can bypass the new required_remote_apis check just by passing a PluginApi instance instead of a string. That reopens undeclared local cross-plugin calls in the exact path this PR is trying to harden.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/plugin/__init__.py` around lines 90 - 93, The current early return
when api_name is a PluginApi bypasses the remote-apis guard; after calling
get_current_remote_apis() you must validate pre-resolved PluginApi inputs
against the same required_remote_apis/current_remote_apis rules as string names.
Modify the branch that handles isinstance(api_name, PluginApi) (and uses
api_name.call_type / ApiType.PROXY) to first consult get_current_remote_apis()
and required_remote_apis and only allow the PluginApi if the remote_apis guard
permits it (otherwise raise or reject); ensure the same validation logic used
for string api_name paths is reused for PluginApi instances.
| active_resolver = resolver or get_current_api_resolver() or get_default_api_resolver() | ||
| if current_remote_apis is not None: | ||
| api = active_resolver.coerce_plugin_api(current_remote_apis.get(api_name)) | ||
| else: | ||
| api = active_resolver.resolve(api_name, None) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "__init__.py" -path "*/framex/plugin/*" | head -20Repository: touale/FrameX-kit
Length of output: 92
🏁 Script executed:
cat -n src/framex/plugin/__init__.py | sed -n '85,110p'Repository: touale/FrameX-kit
Length of output: 1260
🏁 Script executed:
rg -A 3 "def get_current_api_resolver|def get_default_api_resolver" --type pyRepository: touale/FrameX-kit
Length of output: 484
🏁 Script executed:
rg -A 5 "def _resolve_plugin_api" src/framex/plugin/__init__.pyRepository: touale/FrameX-kit
Length of output: 278
🏁 Script executed:
rg -B 2 -A 10 "class ApiResolver" --type py | head -50Repository: touale/FrameX-kit
Length of output: 832
🏁 Script executed:
rg "_manager" src/framex/plugin/__init__.pyRepository: touale/FrameX-kit
Length of output: 496
Ensure active_resolver is always non-None before dereferencing it.
Line 94 produces ApiResolver | None since both get_current_api_resolver() and get_default_api_resolver() return optional types. Lines 96 and 98 will raise AttributeError if both resolver functions return None. Add a fallback to instantiate a default resolver:
Suggested fix
def _resolve_plugin_api(
api_name: str | PluginApi,
resolver: ApiResolver | None = None,
) -> tuple[PluginApi, bool]:
current_remote_apis = get_current_remote_apis()
+ active_resolver = (
+ resolver
+ or get_current_api_resolver()
+ or get_default_api_resolver()
+ or ApiResolver(manager=_manager)
+ )
if isinstance(api_name, PluginApi):
return api_name, api_name.call_type == ApiType.PROXY
- active_resolver = resolver or get_current_api_resolver() or get_default_api_resolver()
if current_remote_apis is not None:
- api = active_resolver.coerce_plugin_api(current_remote_apis.get(api_name))
+ api = ApiResolver.coerce_plugin_api(current_remote_apis.get(api_name))
else:
api = active_resolver.resolve(api_name, None)🧰 Tools
🪛 GitHub Actions: Test Project
[error] 96-96: mypy error: Item "None" of "ApiResolver | None" has no attribute "coerce_plugin_api" [union-attr]
[error] 98-98: mypy error: Item "None" of "ApiResolver | None" has no attribute "resolve" [union-attr]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framex/plugin/__init__.py` around lines 94 - 98, active_resolver may be
None because get_current_api_resolver() and get_default_api_resolver() return
optionals; ensure active_resolver is non-None before using it by providing a
final fallback resolver instance (e.g., construct or call a known default
ApiResolver) when resolver or the two getters return None; modify the assignment
of active_resolver and subsequent uses in this block (references:
active_resolver, resolver, get_current_api_resolver, get_default_api_resolver)
so that coerce_plugin_api and resolve are only called on a guaranteed resolver
object, and keep existing branches using current_remote_apis and api_name
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framex/plugin/resolver.py`:
- Around line 58-60: The setter _set_default_api_resolver currently accepts None
and can clear the global _default_api_resolver causing
get_default_api_resolver() to fail later; change _set_default_api_resolver to
disallow None by updating its signature/typing to accept only ApiResolver (not
ApiResolver | None) and/or add a guard that raises a clear error if resolver is
None, and ensure the global _default_api_resolver assignment always receives a
non-null ApiResolver; update any callers and type hints accordingly to maintain
non-nullability for _default_api_resolver and get_default_api_resolver().
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66d42a51-d98a-496d-aeb7-f051607148c8
📒 Files selected for processing (1)
src/framex/plugin/resolver.py
1 participant
Fix #98
Summary by CodeRabbit
Improvements
New Features
Tests
Chores