Fix/agent tool error handling #529
Conversation
|
Warning Rate limit exceeded@shanthanu47 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 1 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR enhances error handling for tool functions in the chat agent to support both sync and async contexts with improved logging, and adds cross-process locking to repository registration to prevent concurrent write collisions. It also introduces portalocker as a dependency to support the locking mechanism. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical bug in async tool exception handling and adds file locking for concurrent repository registration. The main fix ensures that exceptions in async tools are properly caught and returned as error messages instead of crashing the agent.
Key changes:
- Updated
handle_exceptionwrapper inpydantic_agent.pyto detect and handle async functions usinginspect.iscoroutinefunction() - Added file locking in
register_repo()using portalocker to prevent race conditions during concurrent repository registrations - Added
portalocker==3.1.0as a new dependency for cross-platform file locking
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/modules/intelligence/agents/chat_agents/pydantic_agent.py | Fixed async exception handling by adding coroutine detection and separate async/sync wrappers with proper await statements |
| app/modules/repo_manager/repo_manager.py | Added file locking mechanism to prevent concurrent registration conflicts for the same repository |
| requirements.txt | Added portalocker dependency for cross-platform file locking support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Use portalocker for cross-platform file locking | ||
| # Prevent concurrent registrations for the same repo key | ||
| # if another process is currently registering it. | ||
| with portalocker.Lock(lock_file_path, timeout=10) as locked_file: |
There was a problem hiding this comment.
The variable locked_file returned by the context manager is not used. You can replace as locked_file with just using the context manager without capturing the return value, or use as _ to indicate it's intentionally unused.
| with portalocker.Lock(lock_file_path, timeout=10) as locked_file: | |
| with portalocker.Lock(lock_file_path, timeout=10): |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py(1 hunks)app/modules/repo_manager/repo_manager.py(2 hunks)requirements.txt(1 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
app/modules/repo_manager/repo_manager.py
[warning] 326-326: Remove the unused local variable "locked_file".
🪛 Ruff (0.14.8)
app/modules/repo_manager/repo_manager.py
326-326: Local variable locked_file is assigned to but never used
Remove assignment to unused variable locked_file
(F841)
app/modules/intelligence/agents/chat_agents/pydantic_agent.py
55-55: Do not catch blind exception: Exception
(BLE001)
56-56: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
57-57: Use explicit conversion flag
Replace with conversion flag
(RUF010)
63-63: Do not catch blind exception: Exception
(BLE001)
64-64: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
65-65: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (1)
requirements.txt (1)
176-176: Verify new dependency pin (portalocker==3.1.0) is intentional and compatible.Since this adds a new runtime dependency, please confirm it matches your supported Python versions and deployment environment constraints (and that the exact pinned version is the desired one).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app/modules/repo_manager/repo_manager.py:326
- The variable
locked_fileassigned by the context manager is never used. While the file locking still works correctly, you can simplify the code by not assigning the unused variable.
entry = self._load_metadata_entry(repo_name, branch, commit_id)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| return await tool_func(*args, **kwargs) | ||
| except Exception as e: | ||
| logger.exception("Exception in async tool function", tool_name=tool_func.__name__) |
There was a problem hiding this comment.
The standard Python logging module's exception() method doesn't accept keyword arguments like tool_name. This will raise a TypeError at runtime. Instead, include the tool name in the message string itself.
| try: | ||
| return tool_func(*args, **kwargs) | ||
| except Exception as e: | ||
| logger.exception("Exception in sync tool function", tool_name=tool_func.__name__) |
There was a problem hiding this comment.
The standard Python logging module's exception() method doesn't accept keyword arguments like tool_name. This will raise a TypeError at runtime. Instead, include the tool name in the message string itself.
|
1 participant
Description
This PR fixes a critical bug where
asynctool failures would cause the Agent to crash with an unhandled exception.The Issue:
The
handle_exceptionwrapper inpydantic_agent.pywas synchronous-only. Since most tools areasync, exceptions raised duringawaitwere not being caught by the wrapper.The Fix:
I updated
handle_exceptionto detectinspect.iscoroutinefunction(tool_func)and return anasyncwrapper that properlyawaits the tool execution within thetry/exceptblock.Test Plan:
Verified using a reproduction script that launches an async tool which raises an exception. The new wrapper successfully catches it and returns a descriptive error message to the LLM.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.