Fix trailing slash in agent URL (#768)#774
Conversation
Normalize agent_url by appending a trailing slash in the registration serializer and organization service. This ensures correct URL construction when communicating with agents. Signed-off-by: Sumit-ai-dev <codebysumitdev@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses Issue #768 by ensuring agent_url values are normalized to include a trailing slash so that urllib.parse.urljoin() does not drop path segments when building agent endpoint URLs, preventing broken agent communication.
Changes:
- Normalize
agent_urlwith a trailing slash in the registration serializer validation. - Normalize
agent_urlwith a trailing slash in the organization creation service layer before calling agent endpoints / persisting. - Add unit tests covering trailing-slash normalization and
urljoinbehavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/api-engine/organization/service.py | Adds trailing-slash normalization before constructing agent endpoint URLs with urljoin. |
| src/api-engine/auth/serializers.py | Normalizes agent_url during registration validation to prevent path loss during later urljoin usage. |
| src/api-engine/auth/tests.py | Adds tests demonstrating normalization expectations and the underlying urljoin behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Extract normalize_agent_url to common/utils.py using urllib.parse for safe path-only normalization (preserves query strings/fragments) - Remove duplicate trailing-slash check from _create_organization - Check both normalized and unnormalized forms in uniqueness query - Update tests to import real normalize_agent_url instead of re-implementing it Signed-off-by: Sumit-ai-dev <codebysumitdev@gmail.com>
|
Addressed all 5 points in the latest commit, extracted normalize_agent_url to common/utils.py using urllib.parse (path-only, safe for query strings), removed the duplicate check in _create_organization, updated uniqueness query to cover both normalized and unnormalized forms, and tests now use the real function. Happy to iterate! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Added denormalize_agent_url to common/utils.py using urllib.parse for proper path-aware variant generation (fixes rstrip issue with query strings) - Updated uniqueness check to use denormalize_agent_url instead of rstrip - Added Organization.save() override to normalize agent_url at model level, covering existing DB records on next write - Switched tests from TestCase to SimpleTestCase (no DB access needed) Signed-off-by: Sumit-ai-dev <codebysumitdev@gmail.com>
|
TBH, I don't like this solution. If you normalize the agent URL in the organization model, you don't have to pre-normalize it in the organization service. And, if you normalize the agent URL in the organization service, you don't have to pre-normalize it in the auth serializer. This looks super redundant to me. Moreover, I prefer saving the agent URL whatever users enter rather than normalizing it afterward. This issue should be addressed by fixing the way URL paths are joined in the api engine, not by editing user inputs. |
Addressed maintainer feedback by reverting input normalization and instead fixing the URL construction logic. Created a safe_urljoin utility to guarantee correct path concatenation regardless of the trailing slash in the base agent_url. Updated all service layers to use this new utility. Signed-off-by: Sumit-ai-dev <codebysumitdev@gmail.com>
I've reverted the DB normalization and handled the slash logic at join-time instead. Created a |
I fixed the agent_url trailing slash issue by adding normalization to the registration serializer and the service layer. This ensures urljoin doesn't drop path segments and breaks agent communication.
Verified the logic with unit tests for various URL formats. Happy to iterate if you have feedback!