'fix(sdk): anchor /api path stripping in init.py and tracing.py to avoid corrupting host named api - #6806
'fix(sdk): anchor /api path stripping in init.py and tracing.py to avoid corrupting host named api#6806mihsanalam wants to merge 7 commits into
Conversation
|
@mihsanalam is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
|
|
|
✅ Thanks @mihsanalam! This PR now meets the contribution requirements and has been reopened. A maintainer will review it soon. |
github-actions
Bot
added
the
incomplete-pr
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe SDK now anchors ChangesAPI URL normalization
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The SDK now preserves API hosts and ports while normalizing trailing API paths, with tests covering the affected request URLs. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
github-actions
Bot
removed
the
incomplete-pr
|
@mmabrouk This is ready for review — I've added the demo and 2 commits covering both init.py and tracing.py. Let me know if the CI workflows need approval to run. |
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
thanks for the pr. i found one url edge case that needs a change before this is ready.
| """ | ||
| parts = urlsplit(url) | ||
| path = parts.path.rstrip("/").removesuffix("/api") | ||
| return urlunsplit((parts.scheme, parts.netloc, path, parts.query, parts.fragment)) |
There was a problem hiding this comment.
The preserved query or fragment makes both callers build the wrong URL when they append a path. For example, this returns https://cloud.agenta.ai?tenant=x; AgentaSingleton.init() then creates https://cloud.agenta.ai?tenant=x/api/otlp/v1/traces, whose parsed path is empty and whose query is tenant=x/api/otlp/v1/traces. get_trace_url() breaks the same way, and a fragment swallows the appended path too. Please either clear query/fragment when deriving the host/web base or join the new path through URL components, and add assertions for the final OTLP and trace-view URLs.
There was a problem hiding this comment.
Good catch; confirmed the issue. Preserving query/fragment lets it swallow the appended path (e.g., /w/ws-1/p/proj-1/observability) into the query string instead. Reverted to dropping query/fragment in strip_trailing_api_segment(), added a docstring explaining why, and added two regression tests asserting the final OTLP and get_trace_url() URLs are correct with a query-bearing api_url. Verified both tests genuinely fail without the fix and pass with it. Pushed in mihsanalam@a1219e9
…corrupting concatenated URLs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: c7410215-88d5-431f-97a4-422213dd1f90
📒 Files selected for processing (5)
sdks/python/agenta/sdk/engines/tracing/tracing.pysdks/python/agenta/sdk/utils/helpers.pysdks/python/agenta/sdk/utils/init.pysdks/python/oss/tests/pytest/unit/test_helpers.pysdks/python/oss/tests/pytest/unit/test_init_singleton.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
…request paths built by string concatenation
|
@mmabrouk This is ready for another look. CodeRabbit flagged a related issue on |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
sdks/python/agenta/sdk/utils/init.py (1)
101-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize
hostbefore constructing the fallback API URLWhen
api_urlis omitted andhostcontains a query or fragment,parse_url()preserves it. The fallback then appends/apiafter that component.strip_query_and_fragment()removes the appended/api, and derived OTLP and scope URLs use incorrect paths. Strip the query and fragment from_hostbefore appending/api.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 510c5332-cc93-4648-82c9-f1a15d659c31
📒 Files selected for processing (3)
sdks/python/agenta/sdk/utils/helpers.pysdks/python/agenta/sdk/utils/init.pysdks/python/oss/tests/pytest/unit/test_init_singleton.py
🚧 Files skipped from review as they are similar to previous changes (3)
- sdks/python/agenta/sdk/utils/helpers.py
- sdks/python/agenta/sdk/utils/init.py
- sdks/python/oss/tests/pytest/unit/test_init_singleton.py
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
sdks/python/agenta/sdk/utils/init.py (1)
113-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe new query/fragment normalization preserves a trailing slash after the
/apisegment, so an API URL such ashttps://host/api/?tenant=xmakesresolve_scopes()request/api//projects/currentinstead of/api/projects/current. Normalize the trailing/api/form before storingself.api_urlso scope resolution uses the canonical endpoint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 46804b98-b145-468e-bce5-a520c6ac1631
📒 Files selected for processing (2)
sdks/python/agenta/sdk/utils/init.pysdks/python/oss/tests/pytest/unit/test_init_singleton.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
|
Hi @mmabrouk just checking in. I've addressed every finding from your review and from CodeRabbit across several rounds, all with verified regression tests, and CodeRabbit's latest pass says the change is ready to merge. Let me know if there's anything else needed or if you have a moment to take another look. No rush if you're busy! |
3 participants
Summary
Fixes #6787.
AgentaSingleton.init()derived the SDK's host from the API URL using_api_url.rsplit("/api", 1)[0]— a plain string split that matches/apianywhere in the URL, not just as a path segment. For an internal base URL
whose host is literally
api(e.g.http://api:8000), this matches the//apiinside the scheme/host itself, corrupting the result and droppingthe port. The trace endpoint built from
self.hostthen points at thewrong address, causing the runner to reject it and every turn to fail.
While fixing this, I found the same bug pattern (matching
/apias asubstring instead of anchoring to a path segment) in
tracing.py'sget_trace_url(), which used.replace("/api", "")— actually moreprone to corruption than the original
rsplit, since.replacestripsevery occurrence, not just the last.
The fix extracts a shared
strip_trailing_api_segment()helper intohelpers.py, which parses the URL properly withurlsplit/urlunsplitand only strips a trailing
/apipath segment, leaving scheme/host/port(
parts.netloc) untouched regardless of what the host is named. Bothinit.pyandtracing.pynow use this single helper instead of twoseparate ad-hoc string manipulations. It also fixes a related edge case
found during testing: the original logic in both places silently dropped
any query string or fragment on the URL — the helper now preserves them.
Testing
Verified locally
reported host (
api:8000), a similarly-named host (agenta-api:8000),a normal cloud host, and trailing-slash / query-string / fragment edge
cases.
AgentaSingleton.init()'s hostderivation, plus a regression test confirming
get_trace_url()nolonger corrupts a host named
api.4 failed, 2803 passed, 4 skipped, 3 xfailed.The only 4 failures are the same pre-existing, unrelated ones (litellm
model-ID snapshot drift, a Windows CLI-streaming quirk, and benchmark
markdown encoding), confirmed identical on a clean checkout of main
before this change.
ruff formatandruff checkpass on all changed files with no newissues introduced.
Added or updated tests
sdks/python/oss/tests/pytest/unit/test_helpers.pysdks/python/oss/tests/pytest/unit/test_init_singleton.pyQA follow-up
N/A
Demo
Before (main) — host is corrupted, port dropped:
After (this branch) — host and port preserved correctly:
