Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRemoved the ApiResolver and resolver context machinery, centralized plugin API resolution on Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Ingress as Ingress<br/>(route handler)
participant Proxy as Proxy<br/>Plugin
participant OpenAPI as OpenAPI<br/>Parser
participant Adapter as HTTP<br/>Adapter
Client->>Ingress: POST /proxy/mock/upload (multipart: files + form)
Ingress->>Proxy: forward **request_kwargs (params, data, files)
Proxy->>OpenAPI: consult operation schema (requestBody)
alt multipart/form-data
OpenAPI-->>Proxy: field schemas (mark file vs form)
Proxy->>Proxy: build form_body + files tuples
else application/json
OpenAPI-->>Proxy: single $ref schema
Proxy->>Proxy: build json_body (Pydantic model)
end
Proxy->>Adapter: fetch_response(request_kwargs: data/files/json, params)
Adapter-->>Client: HTTP response (proxied result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/adapter/test_ray_adapter.py (1)
183-191: Good fix to properly dispose of the coroutine.Calling
coro.close()prevents "coroutine was never awaited" warnings. The test correctly verifies the wrapper callsasyncio.runand returns its result.Minor cleanup: The
call_loglist (line 162) and the logging insideasync_funcare now unused since the coroutine is closed without execution. Consider removing the dead code:🧹 Proposed cleanup
- call_log = [] - async def async_func(x): - call_log.append(f"called with {x}") return x * 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/adapter/test_ray_adapter.py` around lines 183 - 191, Remove the now-unused test artifacts: delete the call_log list and any logging inside async_func since the coroutine is closed by _consume_coroutine and never executed; update references in the test around _consume_coroutine and captured_wrapper to ensure no remaining uses of call_log or async_func remain, and run the test to confirm behavior is unchanged (mock_asyncio_run still asserts called and result from captured_wrapper remains 10).src/framex/plugins/proxy/__init__.py (2)
240-241: Remove or uncomment the decorator.Commented-out code suggests incomplete work. If
@logger.catchis intentionally disabled, consider removing it entirely or adding a comment explaining why it's disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/plugins/proxy/__init__.py` around lines 240 - 241, The commented-out decorator `# `@logger.catch`` above the async function `fetch_response` should be resolved: either remove the commented line entirely if it's unnecessary, or uncomment it to re-enable structured error logging; if leaving it disabled is intentional, add a short explanatory comment (e.g., why logging was disabled or conditions under which it should be re-enabled) directly above `fetch_response` so future readers know the rationale.
152-156: Unreachable code branch.This
elseblock can never execute becausecontent_typeis set to either"application/json"or"multipart/form-data"by lines 122-130. Unsupported content types already triggercontinueat line 130.Proposed fix - remove dead code
if is_upload_annotation(annotation): file_param_names.add(field_name) - else: - logger.opt(colors=True).error( - f"Failed to proxy api({method}) <r>{url}{path}</r>, unsupported content type: ${body_content.keys()}" - ) - continue logger.opt(colors=True).trace(f"Found proxy api({method}) <g>{url}{path}</g>")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framex/plugins/proxy/__init__.py` around lines 152 - 156, The else branch that logs "unsupported content type" using logger.opt(...).error and references body_content is unreachable because content_type is already gated to "application/json" or "multipart/form-data" earlier; remove that dead else block (the logger.opt(...).error and continue) from the proxy handling loop in __init__.py and ensure no remaining references to body_content or that unreachable path remain; if you intended to handle other content types instead, replace the removal by adding an explicit fallback branch that inspects content_type and logs the actual unsupported type.
🤖 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/driver/ingress.py`:
- Around line 31-33: Delete the unused helper function _is_basemodel_annotation
(which calls _unwrap_annotation and checks issubclass against BaseModel) from
the file; remove its entire definition to avoid dead code and unused
imports/refs, and ensure no other code references _is_basemodel_annotation
before committing.
In `@src/framex/plugins/proxy/__init__.py`:
- Around line 127-130: The f-string in the logger call uses a stray dollar sign
before {body_content.keys()}, which is invalid Python f-string syntax; in the
proxy logging statement (logger.opt(...).error(...)) remove the `$` so the
expression is interpolated as {body_content.keys()} (i.e., change
"${body_content.keys()}" to "{body_content.keys()}"), keeping the rest of the
message and variables (method, url, path, body_content) intact.
In `@tests/mock.py`:
- Around line 49-62: The files comprehension in the mock response builds file
dicts by unpacking (field_name, file_info) but iterates over files directly,
which yields keys and breaks unpacking; in the elif branch that handles
url.endswith("/proxy/mock/upload") and method == "POST" (inside tests/mock.py)
change the comprehension to iterate over (files or {}).items() so it correctly
gets (field_name, file_info) pairs and handles None safely.
---
Nitpick comments:
In `@src/framex/plugins/proxy/__init__.py`:
- Around line 240-241: The commented-out decorator `# `@logger.catch`` above the
async function `fetch_response` should be resolved: either remove the commented
line entirely if it's unnecessary, or uncomment it to re-enable structured error
logging; if leaving it disabled is intentional, add a short explanatory comment
(e.g., why logging was disabled or conditions under which it should be
re-enabled) directly above `fetch_response` so future readers know the
rationale.
- Around line 152-156: The else branch that logs "unsupported content type"
using logger.opt(...).error and references body_content is unreachable because
content_type is already gated to "application/json" or "multipart/form-data"
earlier; remove that dead else block (the logger.opt(...).error and continue)
from the proxy handling loop in __init__.py and ensure no remaining references
to body_content or that unreachable path remain; if you intended to handle other
content types instead, replace the removal by adding an explicit fallback branch
that inspects content_type and logs the actual unsupported type.
In `@tests/adapter/test_ray_adapter.py`:
- Around line 183-191: Remove the now-unused test artifacts: delete the call_log
list and any logging inside async_func since the coroutine is closed by
_consume_coroutine and never executed; update references in the test around
_consume_coroutine and captured_wrapper to ensure no remaining uses of call_log
or async_func remain, and run the test to confirm behavior is unchanged
(mock_asyncio_run still asserts called and result from captured_wrapper remains
10).
🪄 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: 867f0ed5-90b5-4183-93ae-c219ff64cdf3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
pyproject.tomlpytest.inisrc/framex/driver/ingress.pysrc/framex/plugin/__init__.pysrc/framex/plugin/base.pysrc/framex/plugin/load.pysrc/framex/plugin/model.pysrc/framex/plugin/on.pysrc/framex/plugin/resolver.pysrc/framex/plugins/proxy/__init__.pysrc/framex/plugins/proxy/builder.pytests/adapter/test_ray_adapter.pytests/api/test_proxy.pytests/consts.pytests/driver/test_auth.pytests/mock.pytests/test_plugin.py
💤 Files with no reviewable changes (1)
- src/framex/plugin/load.py
| elif url.endswith("/proxy/mock/upload") and method == "POST": | ||
| resp.json.return_value = { | ||
| "method": "POST", | ||
| "params": params, | ||
| "body": body, | ||
| "files": [ | ||
| { | ||
| "field": field_name, | ||
| "filename": file_info[0], | ||
| "content_type": file_info[2], | ||
| } | ||
| for field_name, file_info in (files or []) | ||
| ], | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Run the specific test to verify if it passes with current implementation
cd tests && python -c "
files = {'ppt_file': ('demo.pptx', b'content', 'application/pptx'), 'txt_file': ('demo.txt', b'txt', 'text/plain')}
try:
result = [(field_name, file_info) for field_name, file_info in files]
print('Direct iteration result:', result)
except Exception as e:
print('Error:', e)
result_items = [(field_name, file_info) for field_name, file_info in files.items()]
print('Using .items():', result_items)
"Repository: touale/FrameX-kit
Length of output: 234
🏁 Script executed:
cat -n tests/mock.py | sed -n '45,65p'Repository: touale/FrameX-kit
Length of output: 917
🏁 Script executed:
cat -n tests/test_proxy.py | sed -n '30,60p'Repository: touale/FrameX-kit
Length of output: 112
🏁 Script executed:
find . -name "*.py" -path "*/tests/*" -type f | head -20Repository: touale/FrameX-kit
Length of output: 600
🏁 Script executed:
rg "proxy/mock/upload" --type pyRepository: touale/FrameX-kit
Length of output: 381
🏁 Script executed:
cat -n tests/api/test_proxy.py | sed -n '1,100p'Repository: touale/FrameX-kit
Length of output: 4171
🏁 Script executed:
cd tests && python -m pytest api/test_proxy.py::test_get_proxy_upload -vRepository: touale/FrameX-kit
Length of output: 245
Fix dict iteration in files comprehension.
When files is passed as a dict (as shown in the test: files={"ppt_file": (...), "txt_file": (...)}), iterating directly over it yields only keys, not (key, value) pairs. The unpacking for field_name, file_info in (files or []) fails with "too many values to unpack". Use .items() instead.
🐛 Proposed fix
elif url.endswith("/proxy/mock/upload") and method == "POST":
resp.json.return_value = {
"method": "POST",
"params": params,
"body": body,
"files": [
{
"field": field_name,
"filename": file_info[0],
"content_type": file_info[2],
}
- for field_name, file_info in (files or [])
+ for field_name, file_info in (files or {}).items()
],
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mock.py` around lines 49 - 62, The files comprehension in the mock
response builds file dicts by unpacking (field_name, file_info) but iterates
over files directly, which yields keys and breaks unpacking; in the elif branch
that handles url.endswith("/proxy/mock/upload") and method == "POST" (inside
tests/mock.py) change the comprehension to iterate over (files or {}).items() so
it correctly gets (field_name, file_info) pairs and handles None safely.
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests
Chores