Bugfix: Incorrect git url in case of GitHub app

[sharoneyal]

User description

Fix incorrect parsing when PR/issue url is from Github app context, so: https://api.github.com/repos/Codium-ai/pr-agent/issues/123 would have generated the incorrect git repo: https://api.github.com/repos/Codium-ai/pr-agent.git instead of https://github.com/Codium-ai/pr-agent.git

---

PR Type

Bug fix

---

Description

• Fix incorrect Git URL generation for GitHub app context.

• Ensure consistent URL formatting for both user and app contexts.

• Improve error handling for invalid or missing URL components.

---

Changes walkthrough 📝

	Relevant files
Bug fix	github_provider.py Fix Git URL generation and error handling                                pr_agent/git_providers/github_provider.py Corrected Git URL generation logic to use base_url_html. Updated logic to ensure proper URL formatting for GitHub app context. Fixed condition checks for missing context in canonical URL parts. Improved error logging for invalid or missing URL components. +3/-3

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-free-for-open-source-projects]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Logic Validation Verify that using base_url_html works correctly for all GitHub contexts (both app and user). The fix changes from dynamically extracting the URL prefix to using a predefined base_url_html property. return f"{self.base_url_html}/{repo_path}.git" #https://github.com / <OWNER>/<REPO>.git Condition Change The condition changed from 'not any()' to 'not all()' which is a significant logical change. Verify this doesn't introduce new edge cases where some but not all parameters are present. if not all([scheme_and_netloc, owner, repo]): #"else": Not invoked from a PR context,but no provided git url for context get_logger().error(f"Unable to get canonical url parts since missing context (PR or explicit git url)")

[sharoneyal]

/improve

[qodo-free-for-open-source-projects]

PR Code Suggestions ✨

Latest suggestions up to 605eef6

Category	Suggestion	Impact
Possible issue	Fix logical condition check The condition is using any() which checks if at least one element is truthy, but the logic requires all elements to be present. Using all() is correct here as we need to ensure all three variables have values. pr_agent/git_providers/github_provider.py [112] -if not any([scheme_and_netloc, owner, repo]): #"else": Not invoked from a PR context,but no provided git url for context +if not all([scheme_and_netloc, owner, repo]): #"else": Not invoked from a PR context,but no provided git url for context Apply this suggestion Suggestion importance[1-10]: 10 __ Why: The PR already implements this exact change, replacing any() with all(). This is a critical fix as the original logic was incorrect - any() would return True if any element was truthy, while the function needs all elements to be present to construct a valid URL.	High
General	Correct implementation for GitHub URLs The new implementation correctly uses self.base_url_html instead of extracting from the input URL, which fixes the issue with GitHub app URLs. This is a good fix for the bug mentioned in the PR title. pr_agent/git_providers/github_provider.py [89] +return f"{self.base_url_html}/{repo_path}.git" #https://github.com / <OWNER>/<REPO>.git - Apply this suggestion Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that using self.base_url_html is better than extracting from the input URL. This is an important fix that ensures consistent URL generation, especially for GitHub app URLs, which aligns with the PR's purpose.	Medium
More

• Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.

---

Previous suggestions

Suggestions up to commit 605eef6

Category	Suggestion	Impact
Learned best practice	Add null safety checks when accessing object attributes to prevent potential runtime errors The code doesn't check if self.base_url_html is defined before using it. This could lead to runtime errors if the attribute is None or not set. Add a null safety check to provide a fallback value when base_url_html is not available. pr_agent/git_providers/github_provider.py [84-89] def get_git_repo_url(self, issues_or_pr_url: str) -> str: repo_path = self._get_owner_and_repo_path(issues_or_pr_url) #Return: <OWNER>/<REPO> if not repo_path or repo_path not in issues_or_pr_url: get_logger().error(f"Unable to retrieve owner/path from url: {issues_or_pr_url}") return "" - return f"{self.base_url_html}/{repo_path}.git" #https://github.com / <OWNER>/<REPO>.git + base_url = getattr(self, 'base_url_html', '') + if not base_url: + get_logger().error("base_url_html is not defined") + return "" + return f"{base_url}/{repo_path}.git" #https://github.com / <OWNER>/<REPO>.git Suggestion importance[1-10]: 6	Low
General	Clarify condition comment The comment is misleading. This condition checks if any required component is missing, not specifically about PR context. Consider updating the comment to accurately reflect the logic. pr_agent/git_providers/github_provider.py [112] -if not all([scheme_and_netloc, owner, repo]): #"else": Not invoked from a PR context,but no provided git url for context +if not all([scheme_and_netloc, owner, repo]): #Check if any required URL component is missing Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that the comment is misleading and doesn't accurately reflect the condition's purpose. The improved comment better explains that the condition checks for missing URL components, which enhances code readability and maintainability.	Low