-
Notifications
You must be signed in to change notification settings - Fork 94
Resolve graph paths relative to configuration file #86
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
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Pull Request Overview
This PR enhances the LangGraphService to resolve graph file paths relative to the configuration file's directory instead of the current working directory. This enables more flexible configuration file placement, particularly when the config lives outside the graphs directory.
Key Changes:
- Store the configuration file's parent directory during initialization
- Resolve relative graph paths against the config directory
- Convert Path objects to strings in
list_graphs()for clearer logging
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _load_graph_registry(self): | ||
| """Load graph definitions from aegra.json""" | ||
| graphs_config = self.config.get("graphs", {}) | ||
| config_dir = self.config_dir or Path.cwd() |
Copilot
AI
Nov 14, 2025
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.
[nitpick] The fallback config_dir = self.config_dir or Path.cwd() is defensive, but _load_graph_registry() is only called from initialize(), where self.config_dir is guaranteed to be set on line 64. The fallback to Path.cwd() could mask bugs where the service is not properly initialized.
Consider either:
- Removing the fallback and trusting that initialization has occurred:
config_dir = self.config_dir
- Adding an assertion to make the requirement explicit:
assert self.config_dir is not None, "Service must be initialized before loading graph registry" config_dir = self.config_dir
This makes the initialization contract clearer and helps catch misuse earlier.
| config_dir = self.config_dir or Path.cwd() | |
| assert self.config_dir is not None, "Service must be initialized before loading graph registry" | |
| config_dir = self.config_dir |
| if ":" not in graph_path: | ||
| raise ValueError(f"Invalid graph path format: {graph_path}") | ||
|
|
||
| file_path, export_name = graph_path.split(":", 1) |
Copilot
AI
Nov 14, 2025
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.
[nitpick] Consider adding a docstring comment above this block to explain the path resolution logic, especially since this is a key feature of the PR. For example:
# Resolve graph file paths relative to config directory.
# Relative paths are resolved against config_dir, absolute paths are used as-is.
resolved_path = Path(file_path)
if not resolved_path.is_absolute():
resolved_path = (config_dir / resolved_path).resolve()This would make the intent clearer for future maintainers.
| file_path, export_name = graph_path.split(":", 1) | |
| file_path, export_name = graph_path.split(":", 1) | |
| # Resolve graph file paths relative to config directory. | |
| # Relative paths are resolved against config_dir, absolute paths are used as-is. |
Summary
Testing