chore(refactor): improve maintability and API documentation#114
Open
slowy07 wants to merge 1 commit into
Open
Conversation
refactor GeoTool registry implementation to improve maintability clarity, and long-term scalability while preserving existing behaviour - Add cached package availability checks using lru_cache. - Add unregister() and require() registry operations Signed-off-by: slowy07 <slowy.arfy@proton.me>
📝 WalkthroughWalkthrough
ChangesGeoToolMeta & GeoToolRegistry API Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@geoagent/core/registry.py`:
- Around line 42-46: In the asdict serialization block, line 45 incorrectly
assigns the list of requires_packages to the requires_confirmation key, causing
the confirmation flag to be overwritten. Change the key on line 45 from
requires_confirmation to requires_packages so that data["requires_packages"] =
list(self.requires_packages) correctly serializes the packages field without
overwriting the confirmation flag.
- Around line 189-190: The condition at lines 189-190 in
geoagent/core/registry.py treats any context value that isn't exactly "fast" as
valid and returns all tool_objects, which is a "fail open" pattern. This means
typos like "Fast" or unknown context values will silently expose every tool
instead of being rejected. Replace the current `if context != "fast"` logic with
explicit validation that only accepts known valid context values (like "fast"
and "full") and raises an error for any unrecognized context, ensuring that
unknown or mistyped contexts fail explicitly rather than defaulting to exposing
all tools.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: c3291f08-d0b1-47d8-b3ab-71a6a6b3cbcc
📒 Files selected for processing (1)
geoagent/core/registry.py
Comment on lines
+42
to
+46
| data = asdict(self) | ||
|
|
||
| data["available_in"] = list(self.available_in) | ||
| data["requires_confirmation"] = list(self.requires_packages) | ||
|
|
There was a problem hiding this comment.
Fix the serializer key for requires_packages.
Line 45 currently overwrites requires_confirmation with the package list, so get_all_tools_config() emits the wrong schema and loses the actual confirmation flag for every tool.
🐛 Proposed fix
data = asdict(self)
data["available_in"] = list(self.available_in)
- data["requires_confirmation"] = list(self.requires_packages)
+ data["requires_packages"] = list(self.requires_packages)
return data🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@geoagent/core/registry.py` around lines 42 - 46, In the asdict serialization
block, line 45 incorrectly assigns the list of requires_packages to the
requires_confirmation key, causing the confirmation flag to be overwritten.
Change the key on line 45 from requires_confirmation to requires_packages so
that data["requires_packages"] = list(self.requires_packages) correctly
serializes the packages field without overwriting the confirmation flag.
Comment on lines
+189
to
+190
| if context != "fast": | ||
| return list(tool_objects) |
There was a problem hiding this comment.
Reject unknown contexts instead of failing open.
Lines 189-190 treat anything except "fast" as "full". A typo like "Fast" or any future restricted context will silently expose every tool instead of preserving the filtered set.
🛡️ Proposed fix
- if context != "fast":
+ if context == "full":
return list(tool_objects)
+ if context != "fast":
+ raise ValueError(f"Unknown tool context '{context}'")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@geoagent/core/registry.py` around lines 189 - 190, The condition at lines
189-190 in geoagent/core/registry.py treats any context value that isn't exactly
"fast" as valid and returns all tool_objects, which is a "fail open" pattern.
This means typos like "Fast" or unknown context values will silently expose
every tool instead of being rejected. Replace the current `if context != "fast"`
logic with explicit validation that only accepts known valid context values
(like "fast" and "full") and raises an error for any unrecognized context,
ensuring that unknown or mistyped contexts fail explicitly rather than
defaulting to exposing all tools.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refactor GeoTool registry implementation to improve maintability clarity, and long-term scalability while preserving existing behaviour
Summary by CodeRabbit
New Features
Improvements