-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Fix Windows Path Handling and Normalize Cross-Platform Path Resolution in AgentLoader #3609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Replace string slicing with os.path.dirname() in config_agent_utils.py for cross-platform path handling - Use pathlib.Path for path normalization in AgentLoader to handle mixed separators - Replace hardcoded '/' with os.sep in error messages for correct path display on Windows - Use os.path.join() for consistent path construction in error messages
Summary of ChangesHello @p-kris10, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves critical path resolution issues within the ADK CLI, particularly on Windows, by implementing a standardized and robust approach to path handling. It replaces error-prone string manipulations with Python's built-in Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses cross-platform path handling issues, particularly for Windows environments, by replacing manual string manipulation with os.path and pathlib utilities. The changes in config_agent_utils.py and agent_loader.py are solid improvements for robustness. The added tests are a great step, but I've identified an issue where one of the new tests will fail on non-Windows platforms and another has a weak assertion. I've provided a suggestion to combine and improve these tests to be platform-aware and more robust.
Add mock-based tests that simulate Windows behavior to catch path handling regressions
|
@Jacksunwei can you take a look at this |
|
Hi @p-kris10 , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Your PR has been received by the team and is currently under review. We will provide feedback as soon as we have an update to share. |
|
Hi @DeanChensj , can you please review this. |
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
Problem:
On Windows, the ADK CLI failed to correctly resolve agent directories due to inconsistent use of hard-coded POSIX separators
/and manual string slicing of paths. This caused incorrect path construction in both functional logic and error messages.Specifically, users encountered errors such as:
Primary issues included:
/separators in path logic and error messagesrsplit("/", 1)/and\) breaking directory resolutionSolution:
This PR standardizes path handling across the ADK CLI to ensure true cross-platform compatibility.
Key improvements:
os.path.dirname()agents_dirusingpathlib.Pathfor consistent behavior on Windows, macOS, and Linuxos.sepandos.path.join()for correct OS-native formatting"C:\\Dev/adk\\subdir/") no longer break resolutionTesting Plan
os.sepusageUnit Tests:
Manual End-to-End (E2E) Tests:
Setup the subagent from yaml example from here on a windows environment and run
adk webthen ask the questionwhat is pi?. The root agent should transfer to math_tutor agent without breaking, the code used to break earlier before the fix.Before :
No output in web ui and following in the terminal
After the fix :
Checklist
Additional context
Add any other context or screenshots about the feature request here.