[SDK] fix(url): keep the path prefix in the user-permissions URL - #7498
Merged
petrotiurin merged 1 commit intoJul 23, 2026
Merged
Conversation
urljoin treats a slash-less final segment as a file and replaces it, so a url_override of http://localhost:5173/api lost /api. Every sibling helper in this module already wraps its base in ensure_ending_slash. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
🟢 size/S
python
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
JetoPistola
added a commit
that referenced
this pull request
Jul 19, 2026
…size-label clobber actions/labeler runs with sync-labels, which writes an authoritative label set computed from a snapshot taken when the action started. The size labeler lived in a separate workflow with its own concurrency group, so the two ran in parallel: labeler read the label set before the size label was applied, then its sync write dropped it. On PR #7498 the size step added 🟢 size/S at 14:23:15 while labeler (started 14:23:14, finished 14:23:17) synced it away, leaving the PR with no size label. Merge the size labeler into the Auto Label PR workflow as a second job that needs the labeler job, so it runs strictly after the sync completes and can no longer be clobbered. size-labeler uses if: !cancelled() so a labeler failure (e.g. a flaky action download) does not skip size labeling, while a superseded run is still skipped. Both jobs share the workflow's single concurrency group and stay independently re-runnable. Keep sync-labels: true — ordering, not disabling it, removes the race, so stale path labels still get cleaned up. Reconcile the size bucket with an incremental delta (remove non-target managed buckets, add the target if missing) rather than a whole-set setLabels write: now that ordering serializes the writers, atomicity is unnecessary, and a delta only ever touches size labels so it can't clobber a label added by anyone else. Read live labels rather than the stale event payload, and log each mutation so a run's output shows what it actually did. Bump actions/labeler v5 -> v6 to clear the Node 20 deprecation warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JetoPistola
added a commit
that referenced
this pull request
Jul 19, 2026
…size-label clobber actions/labeler runs with sync-labels, which writes an authoritative label set computed from a snapshot taken when the action started. The size labeler lived in a separate workflow with its own concurrency group, so the two ran in parallel: labeler read the label set before the size label was applied, then its sync write dropped it. On PR #7498 the size step added 🟢 size/S at 14:23:15 while labeler (started 14:23:14, finished 14:23:17) synced it away, leaving the PR with no size label. Merge the size labeler into the Auto Label PR workflow as a second job that needs the labeler job, so it runs strictly after the sync completes and can no longer be clobbered. size-labeler uses if: !cancelled() so a labeler failure (e.g. a flaky action download) does not skip size labeling, while a superseded run is still skipped. Both jobs share the workflow's single concurrency group and stay independently re-runnable. Keep sync-labels: true — ordering, not disabling it, removes the race, so stale path labels still get cleaned up. Reconcile the size bucket with an incremental delta (remove non-target managed buckets, add the target if missing) rather than a whole-set setLabels write: now that ordering serializes the writers, atomicity is unnecessary, and a delta only ever touches size labels so it can't clobber a label added by anyone else. Read live labels rather than the stale event payload, and log each mutation so a run's output shows what it actually did. Bump actions/labeler v5 -> v6 to clear the Node 20 deprecation warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JetoPistola
added a commit
that referenced
this pull request
Jul 20, 2026
…size-label clobber (#7502) actions/labeler runs with sync-labels, which writes an authoritative label set computed from a snapshot taken when the action started. The size labeler lived in a separate workflow with its own concurrency group, so the two ran in parallel: labeler read the label set before the size label was applied, then its sync write dropped it. On PR #7498 the size step added 🟢 size/S at 14:23:15 while labeler (started 14:23:14, finished 14:23:17) synced it away, leaving the PR with no size label. Merge the size labeler into the Auto Label PR workflow as a second job that needs the labeler job, so it runs strictly after the sync completes and can no longer be clobbered. size-labeler uses if: !cancelled() so a labeler failure (e.g. a flaky action download) does not skip size labeling, while a superseded run is still skipped. Both jobs share the workflow's single concurrency group and stay independently re-runnable. Keep sync-labels: true — ordering, not disabling it, removes the race, so stale path labels still get cleaned up. Reconcile the size bucket with an incremental delta (remove non-target managed buckets, add the target if missing) rather than a whole-set setLabels write: now that ordering serializes the writers, atomicity is unnecessary, and a delta only ever touches size labels so it can't clobber a label added by anyone else. Read live labels rather than the stale event payload, and log each mutation so a run's output shows what it actually did. Bump actions/labeler v5 -> v6 to clear the Node 20 deprecation warning. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
petrotiurin
approved these changes
Jul 23, 2026
petrotiurin
left a comment
petrotiurin
left a comment
Contributor
There was a problem hiding this comment.
Thank you for fixing this issue! Looks good to me.
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
2 participants
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.
Details
get_user_permissions_urldoesn't wrap its base inensure_ending_slash, sourljointreats the slash-less final segment as a file and replaces it — dropping the/apiprefix:Every sibling helper in this module already does wrap its base —
get_is_alive_ping_url(:105),get_experiment_url_by_id(:39),get_project_url_by_trace_id(:60). This one is the lone outlier:The no-trailing-slash form isn't an edge case — it's the documented way to configure this, and what this repo's own tests use (
tests/conftest.py:229setsOPIK_URL_OVERRIDE=http://localhost:5173/api). The in-code defaultsOPIK_URL_CLOUD/OPIK_URL_LOCAL(config.py:29-30) do carry a trailing slash, which is why the shipped default masks this — it only bites users who seturl_overridethemselves.Impact is diagnostic-path only:
opik healthcheck→check_user_permissions.run()→list_user_permissions()→ this helper, so the request 404s and the healthcheck reportsFailed to fetch user permissions: HTTP error 404, pointing the user at their server or credentials rather than at this. No data loss.Change checklist
Issues
No existing issue — happy to open one if the team prefers that order for small SDK fixes.
Testing
Added
test_get_user_permissions_url__url_override__keeps_path_prefix, parametrized over both slash forms of both the local and cloud URLs — matching how the siblingtest_get_is_alive_ping_url__base_url__returns_expected_ping_urlright above it already pins that contract for its own helper.Verified it fails first: the two no-trailing-slash cases fail before the fix while the two trailing-slash cases pass, which is exactly the shape of the bug. All 17 pass after.
tests/unit/test_url_helpers.py17 passed;tests/unit/{message_processing,rate_limit,anonymizer}471 passed alongside it. (One timing test,test_streamer__flush__attachment_uploads__timeout[0.5], flaked under parallel load — it passes on its own both with and without this change; I checked rather than assumed.)ruff checkandruff format --checkclean.Documentation
None needed — this restores the documented behaviour.
AI-WATERMARK: yes
Tools: Claude Code
Model(s): Claude Opus 4.8
Scope: Bug located with AI assistance and the fix drafted with it. I reproduced the dropped prefix myself against all four URL forms, read the sibling helpers to confirm this one is the outlier, verified the no-slash form is what the repo's own conftest uses, and ran the tests and the flake check above myself.