Skip to content

Conversation

@evanscastonguay
Copy link

@evanscastonguay evanscastonguay commented Jan 10, 2026

User description

Summary

  • Avoid raising for non-GitHub providers
  • Publish a friendly message when /similar_issue is invoked outside GitHub
  • Add unit tests for non-GitHub behavior

Testing

  • python3 -m pytest tests/unittest/test_similar_issue_non_github.py

PR Type

Bug fix, Tests


Description

  • Replace exception with graceful handling for non-GitHub providers

  • Publish friendly message when /similar_issue invoked outside GitHub

  • Add comprehensive unit tests for non-GitHub behavior


Diagram Walkthrough

flowchart LR
  A["Non-GitHub Provider Detected"] --> B["Set supported flag to false"]
  B --> C["Return early from run method"]
  C --> D{publish_output enabled?}
  D -->|Yes| E["Publish friendly message"]
  D -->|No| F["Return empty string"]
  E --> F
Loading

File Walkthrough

Relevant files
Bug fix
pr_similar_issue.py
Graceful non-GitHub provider handling with messaging         

pr_agent/tools/pr_similar_issue.py

  • Changed exception raising to graceful handling for non-GitHub
    providers
  • Added supported flag to track GitHub availability
  • Early return in __init__ for unsupported providers
  • Added message publishing logic in run() method when provider
    unsupported
  • Attempts to publish user-friendly message if publish_output is enabled
+18/-3   
Tests
test_similar_issue_non_github.py
Unit tests for non-GitHub provider behavior                           

tests/unittest/test_similar_issue_non_github.py

  • New test file with two async test cases
  • Test verifies message publication when publish_output is enabled
  • Test verifies silent return when publish_output is disabled
  • Uses monkeypatch to mock settings and git provider behavior
+49/-0   

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Unsafe dynamic import

Description: Dynamic import of get_git_provider_with_context inside exception handler may mask import
errors and make debugging difficult, potentially hiding security-relevant failures in
provider initialization.
pr_similar_issue.py [265-265]

Referred Code
from pr_agent.git_providers import get_git_provider_with_context
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Consistent Naming Conventions

Objective: All new variables, functions, and classes must follow the project's established naming
standards

Status: Passed

No Dead or Commented-Out Code

Objective: Keep the codebase clean by ensuring all submitted code is active and necessary

Status: Passed

Robust Error Handling

Objective: Ensure potential errors and edge cases are anticipated and handled gracefully throughout
the code

Status: Passed

Single Responsibility for Functions

Objective: Each function should have a single, well-defined responsibility

Status: Passed

When relevant, utilize early return

Objective: In a code snippet containing multiple logic conditions (such as 'if-else'), prefer an
early return on edge cases than deep nesting

Status: Passed

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid creating partially initialized objects

In the init method, initialize all instance attributes to None for
unsupported providers to prevent creating partially initialized objects and
improve code robustness.

pr_agent/tools/pr_similar_issue.py [18-29]

 class PRSimilarIssue:
     def __init__(self, issue_url: str, ai_handler, args: list = None):
--        if get_settings().config.git_provider != "github":
--            raise Exception("Only github is supported for similar issue tool")
-+        self.issue_url = issue_url
-+        self.supported = get_settings().config.git_provider == "github"
-+        if not self.supported:
-+            return
+        self.issue_url = issue_url
+        self.supported = get_settings().config.git_provider == "github"
+        if not self.supported:
+            self.cli_mode = None
+            self.max_issues_to_scan = None
+            self.git_provider = None
+            self.repo_name = None
+            self.issue_main = None
+            self.issue_body = None
+            self.issue_title = None
+            self.ai_handler = None
+            self.token_handler = None
+            return
 
-         self.cli_mode = get_settings().CONFIG.CLI_MODE
-         self.max_issues_to_scan = get_settings().pr_similar_issue.max_issues_to_scan
--        self.issue_url = issue_url
-         self.git_provider = get_git_provider()()
-         repo_name, issue_number = self.git_provider._parse_issue_url(issue_url.split('=')[-1])
-         self.git_provider.repo = repo_name
+        self.cli_mode = get_settings().CONFIG.CLI_MODE
+        self.max_issues_to_scan = get_settings().pr_similar_issue.max_issues_to_scan
+        self.git_provider = get_git_provider()()
+        repo_name, issue_number = self.git_provider._parse_issue_url(issue_url.split('=')[-1])
+        self.git_provider.repo = repo_name
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that returning early from __init__ creates a partially-initialized object, which is a poor design practice and could lead to future AttributeError bugs.

Medium
General
Improve test by mocking configuration directly

Refactor the test test_similar_issue_non_github_publishes_message to directly
mock configuration values on the global_settings object instead of mocking the
get_settings function, making the test less brittle.

tests/unittest/test_similar_issue_non_github.py [6-34]

 @pytest.mark.asyncio
 async def test_similar_issue_non_github_publishes_message(monkeypatch):
     class FakeProvider:
         def __init__(self):
             self.comments = []
 
         def publish_comment(self, body):
             self.comments.append(body)
 
     fake_provider = FakeProvider()
 
-    class FakeSettings:
-        class config:
-            git_provider = "gitlab"
-            publish_output = True
+    monkeypatch.setattr("pr_agent.config_loader.global_settings.config.git_provider", "gitlab")
+    monkeypatch.setattr("pr_agent.config_loader.global_settings.config.publish_output", True)
 
-    monkeypatch.setattr("pr_agent.tools.pr_similar_issue.get_settings", lambda: FakeSettings)
     monkeypatch.setattr(
         "pr_agent.git_providers.get_git_provider_with_context",
         lambda _: fake_provider,
     )
 
     tool = PRSimilarIssue("https://gitlab.example.com/group/repo/-/merge_requests/1", None)
     result = await tool.run()
 
     assert result == ""
     assert fake_provider.comments == [
         "The /similar_issue tool is currently supported only for GitHub."
     ]
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that mocking get_settings to return a class is brittle and proposes a more robust approach by directly patching the global_settings object, improving test quality.

Low
Learned
best practice
Cache configuration values at initialization

The get_settings().config.publish_output is called in the run() method. Consider
storing this value during initialization to avoid repeated function calls.

pr_agent/tools/pr_similar_issue.py [21-263]

 self.supported = get_settings().config.git_provider == "github"
+self.publish_output = get_settings().config.publish_output
 if not self.supported:
     return
 ...
 async def run(self):
     if not self.supported:
         message = "The /similar_issue tool is currently supported only for GitHub."
-        if get_settings().config.publish_output:
+        if self.publish_output:

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Avoid repeatedly calling configuration or settings retrieval functions inside loops or multiple times. Fetch configuration values once and reuse the stored value to improve performance.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant