-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(seer): use URL-safe GitLab path when building owner/name #118054
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
Changes from 2 commits
2c80ced
8c0afd5
2882f02
fbba9cc
fc8fccb
4230854
8d4bbeb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -498,14 +498,32 @@ def clear_preference_automation_handoff(project: Project) -> None: | |
| ).delete() | ||
|
|
||
|
|
||
| def get_repo_url_path(repo: Repository) -> str: | ||
| """Return the URL-safe owner/name path for a repository. | ||
|
|
||
| For GitLab, ``repo.name`` is ``name_with_namespace`` (the human-readable | ||
| display name, e.g. ``"My Group / My Project"`` — with spaces). The | ||
| URL-safe equivalent is stored in ``repo.config["path"]`` | ||
| (``path_with_namespace``, e.g. ``"my-group/my-project"``). | ||
|
|
||
| For GitHub and all other providers, ``repo.name`` is already the | ||
| URL-safe ``owner/repo`` string, so we return it unchanged. | ||
| """ | ||
| if repo.provider == "integrations:gitlab": | ||
| path = repo.config.get("path") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would we rather raise if there's no path as that's unexpected
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah good point, will change to throw. dont think we need a metric, ive looked at the db and there are no cases where path doesnt exist |
||
| if path: | ||
| return path | ||
| return repo.name | ||
|
|
||
|
|
||
| def build_repo_definition_from_project_repo( | ||
| seer_project_repo: SeerProjectRepository, | ||
| ) -> SeerRepoDefinition | None: | ||
| """Build a SeerRepoDefinition from a SeerProjectRepository with its joined Repository. | ||
|
|
||
| Returns None if Repository name is invalid.""" | ||
| repo = seer_project_repo.project_repository.repository | ||
| repo_name_sections = repo.name.split("/") | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this has downstream (seer) implications since the repo definition is used in seer but i believe we already have utils in seer that strip whitespace, etc |
||
| repo_name_sections = get_repo_url_path(repo).split("/") | ||
|
sentry[bot] marked this conversation as resolved.
|
||
| if len(repo_name_sections) < 2: | ||
| sentry_sdk.capture_exception(ValueError(f"Invalid repository name format: {repo.name}")) | ||
| return None | ||
|
|
@@ -839,7 +857,7 @@ def get_autofix_repos_from_project_code_mappings( | |
| repos: dict[tuple, dict] = {} | ||
| for code_mapping in code_mappings: | ||
| repo: Repository = code_mapping.project_repository.repository | ||
| repo_name_sections = repo.name.split("/") | ||
| repo_name_sections = get_repo_url_path(repo).split("/") | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| if ( | ||
| # We expect a repository name to be in the format of "owner/name" for now. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
| from sentry.models.repository import Repository | ||
| from sentry.seer.autofix.utils import ( | ||
| add_seer_project_repos, | ||
| get_repo_url_path, | ||
| replace_all_seer_project_repos, | ||
| ) | ||
| from sentry.seer.models.project_repository import ( | ||
|
|
@@ -69,9 +70,9 @@ class ProjectRepoResponse(TypedDict): | |
|
|
||
| def _serialize_project_repo(project_repo: SeerProjectRepository) -> ProjectRepoResponse: | ||
| repo = project_repo.project_repository.repository | ||
| name_parts = repo.name.split("/", 1) | ||
| name_parts = get_repo_url_path(repo).split("/", 1) | ||
| owner = name_parts[0] if len(name_parts) > 1 else "" | ||
| name = name_parts[1] if len(name_parts) > 1 else repo.name | ||
| name = name_parts[1] if len(name_parts) > 1 else get_repo_url_path(repo) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this changes our api response, which could be unexpected if users expect to see their display name... this is more correct though since we assume that this is used as a "slug" like we do for github. |
||
|
|
||
| return ProjectRepoResponse( | ||
| id=str(project_repo.id), | ||
|
|
||
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.
GitLab path name lookup mismatch
Medium Severity
get_repository_definitionstill resolves repositories by exactRepository.name, while this change makes GitLabowner/namecome fromget_repo_url_path(config["path"]). Callers that rebuildrepo_full_namefrom those slug fields (withoutexternal_id) no longer match GitLab rows stored undername_with_namespacedisplay names.Additional Locations (1)
src/sentry/seer/autofix/utils.py#L535-L547Reviewed by Cursor Bugbot for commit bb78554. Configure here.
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.
Clanker checked (including seer):
I checked the three things that could break, and traced each through the actual Seer code (../seer):
GitLab API access — unaffected. Seer addresses GitLab repos exclusively by external_id (parsed as {netloc}:{project_id} at
scm/providers/gitlab/provider.py:238), never by owner/name. external_id is unchanged by this PR.
Web/PR/commit URLs — fixed. web_repository_path does self.repository["name"].replace(" ", "") (provider.py:247). The old display name "My Group / My
Project" collapsed to "MyGroup/MyProject" — wrong case, no dashes → the exact 404 in your branch name. The new slug "my-group/my-project" makes that
.replace() a no-op and the URL correct.
external_id discovery — improved. Seer's get_external_id_from_user_org_context (explorer/tools/utils.py:46-78) matches context repos against the LLM's
clean slug via _normalize_repo_full_name, which only trims whitespace around / (not case/dashes). Before, context held display names → "My Group/My Project"
≠ "my-group/my-project" → match often failed. Now the context repos come from the same get_autofix_repos_from_project_code_mappings you changed, so both
sides are clean slugs → they match → external_id is reliably found.