Support Bitbucket Basic Auth #1683

wirelessr · 2025-04-08T09:59:33Z

User description

#1681

Enhance Bitbucket provider functionality and update secret configuration template.

PR Type

Enhancement

Description

Added support for configurable authentication header in Bitbucket provider.
Introduced auth_type to switch between "bearer" and "basic" auth.
Updated _prepare_clone_url_with_token to handle both auth methods.
Enhanced .secrets_template.toml to include new auth configuration options.

Changes walkthrough 📝

Relevant files

Enhancement

bitbucket_provider.py `Add support for configurable Bitbucket authentication` pr_agent/git_providers/bitbucket_provider.py Added logic to support "basic" and "bearer" authentication types. Improved error handling for missing tokens or invalid auth types. Updated `_prepare_clone_url_with_token` to handle both auth methods. Refactored authentication initialization for better clarity.	+30/-17

Configuration changes

.secrets_template.toml `Update secrets template for Bitbucket auth configuration` pr_agent/settings/.secrets_template.toml Added `auth_type` configuration for Bitbucket authentication. Included `basic_token` for "basic" authentication. Updated documentation for new auth options.	+5/-1

Need help?
Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
Check out the documentation for more information.

…ion template.

qodo-merge-for-open-source · 2025-04-08T09:59:56Z

PR Reviewer Guide 🔍

(Review updated until commit `ca95e87`)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 1681 - Fully compliant Compliant requirements: • Make the auth header scheme configurable for Bitbucket integration • Support Basic Auth headers instead of hardcoded Bearer tokens • Allow users to modify authentication from "Bearer {bearer}" to "Basic {bearer}"
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Error Handling The error handling in the initialization could be improved. If authentication fails, the code still sets self.headers and initializes the bitbucket_client, which might lead to unexpected behavior later. except Exception as e: get_logger().exception(f"Failed to initialize Bitbucket authentication: {e}") raise Code Duplication The token validation logic is duplicated for both auth types. Consider refactoring to reduce duplication and improve maintainability. self.basic_token = context.get("bitbucket_basic_token", None) or get_settings().get("BITBUCKET.BASIC_TOKEN", None) if not self.basic_token: raise ValueError("Basic auth requires a token") s.headers["Authorization"] = f"Basic {self.basic_token}" else: # default to bearer self.bearer_token = context.get("bitbucket_bearer_token", None) or get_settings().get("BITBUCKET.BEARER_TOKEN", None) if not self.bearer_token: raise ValueError("Bearer token is required for bearer auth") s.headers["Authorization"] = f"Bearer {self.bearer_token}"

qodo-merge-for-open-source · 2025-04-08T10:00:20Z

PR Code Suggestions ✨

Latest suggestions up to c0c3075

Category	Suggestion	Impact
General	Fix inconsistent indentation Fix the indentation in the `else` clause to maintain consistent code style. The current indentation has an extra space which could cause confusion. pr_agent/git_providers/bitbucket_provider.py [47-48] elif self.auth_type == "bearer": self.bearer_token = context.get("bitbucket_bearer_token", None) or get_settings().get("BITBUCKET.BEARER_TOKEN", None) if not self.bearer_token: raise ValueError("Bearer token is required for bearer auth") s.headers["Authorization"] = f"Bearer {self.bearer_token}" else: - raise ValueError(f"Unsupported auth_type: {self.auth_type}") + raise ValueError(f"Unsupported auth_type: {self.auth_type}") `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an indentation inconsistency in the else clause (line 48) which has an extra space compared to the rest of the code. This is a valid style issue that could potentially cause confusion or maintenance problems in Python, where indentation is syntactically significant.	Medium
Update

Previous suggestions

✅ Suggestions up to commit ca95e87

Category	Suggestion	Impact
Possible issue	Fix basic auth URL format The clone URL format for basic authentication is incorrect. For basic auth, you should use the username:password format in the URL, not the x-token-auth approach which is for bearer tokens. pr_agent/git_providers/bitbucket_provider.py [626-631] if hasattr(self, 'basic_token'): # Basic auth with token - clone_url = f"{scheme}x-token-auth:{self.basic_token}@bitbucket.org{base_url}" + clone_url = f"{scheme}{self.basic_token}@bitbucket.org{base_url}" elif hasattr(self, 'bearer_token'): # Bearer token clone_url = f"{scheme}x-token-auth:{self.bearer_token}@bitbucket.org{base_url}" Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the URL format for basic authentication should be different from bearer token authentication. Using the proper authentication format is critical for the clone operation to work correctly with basic auth, making this a high-impact fix.	Medium
Learned best practice	✅ ~~Extract repeated token retrieval logic into a helper function to reduce code duplication~~ Suggestion Impact: The commit implemented the suggestion by creating a helper function 'get_token' that extracts the repeated token retrieval logic, reducing code duplication. The implementation is very similar to the suggestion with minor differences in variable naming (auth_type vs self.auth_type) and additional error handling for unsupported auth types. code diff: + self.auth_type = context.get("bitbucket_auth_type", None) or get_settings().get("BITBUCKET.AUTH_TYPE", "bearer") + + def get_token(token_name, auth_type_name): + token = context.get(f"bitbucket_{token_name}", None) or get_settings().get(f"BITBUCKET.{token_name.upper()}", None) + if not token: + raise ValueError(f"{auth_type_name} auth requires a token") + return token + + if self.auth_type == "basic": + self.basic_token = get_token("basic_token", "Basic") s.headers["Authorization"] = f"Basic {self.basic_token}" - else: # default to bearer - self.bearer_token = context.get("bitbucket_bearer_token", None) or get_settings().get("BITBUCKET.BEARER_TOKEN", None) - if not self.bearer_token: - raise ValueError("Bearer token is required for bearer auth") + elif self.auth_type == "bearer": + self.bearer_token = get_token("bearer_token", "Bearer") s.headers["Authorization"] = f"Bearer {self.bearer_token}" + else: + raise ValueError(f"Unsupported auth_type: {self.auth_type}") The authentication code has repeated patterns for retrieving tokens from context or settings and setting headers. Extract this logic into a helper method to reduce duplication and make the code more maintainable. pr_agent/git_providers/bitbucket_provider.py [34-46] try: auth_type = context.get("bitbucket_auth_type", None) or get_settings().get("BITBUCKET.AUTH_TYPE", "bearer") - + + def get_token(token_name, auth_type_name): + token = context.get(f"bitbucket_{token_name}", None) or get_settings().get(f"BITBUCKET.{token_name.upper()}", None) + if not token: + raise ValueError(f"{auth_type_name} auth requires a token") + return token + if auth_type == "basic": - self.basic_token = context.get("bitbucket_basic_token", None) or get_settings().get("BITBUCKET.BASIC_TOKEN", None) - if not self.basic_token: - raise ValueError("Basic auth requires a token") + self.basic_token = get_token("basic_token", "Basic") s.headers["Authorization"] = f"Basic {self.basic_token}" else: # default to bearer - self.bearer_token = context.get("bitbucket_bearer_token", None) or get_settings().get("BITBUCKET.BEARER_TOKEN", None) - if not self.bearer_token: - raise ValueError("Bearer token is required for bearer auth") + self.bearer_token = get_token("bearer_token", "Bearer") s.headers["Authorization"] = f"Bearer {self.bearer_token}" Suggestion importance[1-10]: 6	Low

✅ Suggestions up to commit ca95e87

Category	Suggestion	Impact
Learned best practice	✅ ~~Add null safety checks before using authentication tokens to prevent potential runtime errors~~ Suggestion Impact: The commit changed the approach to authentication validation by checking auth_type instead of using hasattr(), which is a different implementation but achieves the same goal of preventing runtime errors when accessing tokens. The code now checks if auth_type is 'basic' or 'bearer' before using the respective tokens. code diff: - if hasattr(self, 'basic_token'): + if self.auth_type == "basic": # Basic auth with token clone_url = f"{scheme}x-token-auth:{self.basic_token}@bitbucket.org{base_url}" - elif hasattr(self, 'bearer_token'): + elif self.auth_type == "bearer": # Bearer token clone_url = f"{scheme}x-token-auth:{self.bearer_token}@bitbucket.org{base_url}" else: - get_logger().error("No valid authentication method provided. Returning None") + # This case should ideally not be reached if __init__ validates auth_type + get_logger().error(f"Unsupported or uninitialized auth_type: {getattr(self, 'auth_type', 'N/A')}. Returning None") return None The code directly uses `self.basic_token` and `self.bearer_token` without checking if they're valid (not None or empty). Even though there are checks in the `init` method, it's better to add defensive checks here as well to prevent potential runtime errors if the attributes are somehow not set or are empty. pr_agent/git_providers/bitbucket_provider.py [628] +if not self.basic_token: + get_logger().error("Basic token is empty or None. Returning None") + return None clone_url = f"{scheme}x-token-auth:{self.basic_token}@bitbucket.org{base_url}" Suggestion importance[1-10]: 6	Low

✅ Suggestions up to commit ca95e87

Category	Suggestion	Impact
Possible issue	Fix authentication in URL The authentication mechanism in the clone URL is incorrect. For Bitbucket, both basic and bearer tokens should use the same URL format with "x-token-auth" as the username. However, the authentication type should be reflected in how the token is used during API calls, not in the clone URL construction. pr_agent/git_providers/bitbucket_provider.py [626-631] -if hasattr(self, 'basic_token'): - # Basic auth with token - clone_url = f"{scheme}x-token-auth:{self.basic_token}@bitbucket.org{base_url}" -elif hasattr(self, 'bearer_token'): - # Bearer token - clone_url = f"{scheme}x-token-auth:{self.bearer_token}@bitbucket.org{base_url}" +# Both auth types use the same clone URL format +token = getattr(self, 'basic_token', None) or getattr(self, 'bearer_token', None) +clone_url = f"{scheme}x-token-auth:{token}@bitbucket.org{base_url}" Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that both authentication types should use the same URL format for cloning. The improved code simplifies the logic by using getattr() to retrieve whichever token is available, eliminating redundant code and making the implementation more maintainable.	Medium

pythonclcoding · 2025-04-09T06:34:04Z

/describe

qodo-merge-for-open-source · 2025-04-09T06:34:14Z

Title

Support Bitbucket Basic Auth

User description

#1681

Enhance Bitbucket provider functionality and update secret configuration template.

PR Type

Enhancement

Description

Added support for configurable authentication headers in Bitbucket provider.
Introduced auth_type to select between "bearer" and "basic" authentication.
Updated _prepare_clone_url_with_token to handle both auth types.
Enhanced .secrets_template.toml to include new configuration options.

Changes walkthrough 📝

Relevant files

Enhancement

bitbucket_provider.py `Support configurable authentication in Bitbucket provider` pr_agent/git_providers/bitbucket_provider.py Added support for "basic" and "bearer" authentication types. Implemented error handling for missing tokens in both auth types. Updated `_prepare_clone_url_with_token` to support both auth methods. Improved logging for authentication initialization errors.	+30/-17

Configuration changes

.secrets_template.toml `Update secrets template for Bitbucket auth configuration` pr_agent/settings/.secrets_template.toml Added `auth_type` configuration for Bitbucket authentication. Included `basic_token` for "basic" authentication. Updated comments to clarify usage of new settings.	+5/-1

Need help?
Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
Check out the documentation for more information.

pythonclcoding · 2025-04-09T06:35:27Z

/review

qodo-merge-for-open-source · 2025-04-09T06:35:46Z

Persistent review updated to latest commit ca95e87

pythonclcoding · 2025-04-09T06:39:30Z

/improve

pr_agent/git_providers/bitbucket_provider.py

mrT23 · 2025-04-11T06:54:57Z

/improve

Co-authored-by: Prateek <110811408+Prateikx@users.noreply.github.com>

mrT23 · 2025-04-11T06:57:58Z

Extract repeated token retrieval logic into a helper function to reduce code duplication

The authentication code has repeated patterns for retrieving tokens from context
or settings and setting headers. Extract this logic into a helper method to
reduce duplication and make the code more maintainable.

pr_agent/git_providers/bitbucket_provider.py [34-46]

 try:
     auth_type = context.get("bitbucket_auth_type", None) or get_settings().get("BITBUCKET.AUTH_TYPE", "bearer")
-
+    
+    def get_token(token_name, auth_type_name):
+        token = context.get(f"bitbucket_{token_name}", None) or get_settings().get(f"BITBUCKET.{token_name.upper()}", None)
+        if not token:
+            raise ValueError(f"{auth_type_name} auth requires a token")
+        return token
+        
     if auth_type == "basic":
-        self.basic_token = context.get("bitbucket_basic_token", None) or get_settings().get("BITBUCKET.BASIC_TOKEN", None)
-        if not self.basic_token:
-            raise ValueError("Basic auth requires a token")
+        self.basic_token = get_token("basic_token", "Basic")
         s.headers["Authorization"] = f"Basic {self.basic_token}"
     else:  # default to bearer
-        self.bearer_token = context.get("bitbucket_bearer_token", None) or get_settings().get("BITBUCKET.BEARER_TOKEN", None)
-        if not self.bearer_token:
-            raise ValueError("Bearer token is required for bearer auth")
+        self.bearer_token = get_token("bearer_token", "Bearer")
         s.headers["Authorization"] = f"Bearer {self.bearer_token}"

Suggestion importance[1-10]: 6

@wirelessr please implement this. it will make reading the code easier

Co-authored-by: Prateek <110811408+Prateikx@users.noreply.github.com>

mrT23 · 2025-04-11T07:00:00Z

@wirelessr thanks for the PR.
in addition to the code suggestion I made above:

please validate that indeed the code works, and you are able to authenticate (in both ways)
could you add a couple of lines to the documentation for this PR ? it will help other users to utilize this option:
https://github.com/qodo-ai/pr-agent/blob/main/docs/docs/installation/bitbucket.md

…ce code duplication

wirelessr · 2025-04-11T09:49:58Z

@mrT23
I've modified the way the bearer token is obtained to be compatible with past code and verified that both bearer and basic work fine as follows.

2025-04-11 09:42:10.595 | INFO     | pr_agent.agent.pr_agent:handle_request:81 - User has set the response language to: zh-TW
2025-04-11 09:42:24.826 | INFO     | pr_agent.tools.pr_reviewer:run:131 - Reviewing PR: https://bitbucket.org/xxx/xxx/123 ...
2025-04-11 09:42:24.840 | INFO     | pr_agent.algo.pr_processing:get_pr_diff:63 - PR main language: Other
2025-04-11 09:42:24.851 | INFO     | pr_agent.algo.pr_processing:get_pr_diff:74 - Tokens: 2613, total tokens under limit: 32000, returning full diff.

federoitman · 2025-04-11T20:44:16Z

I believe this code broke passing the BEARER token to the pr-agent docker image. I get "ValueError: Failed to get git provider for...."

mrT23 · 2025-04-12T07:11:34Z

@federoitman
thanks. fixed
8bdd116

federoitman · 2025-04-14T12:16:46Z

Thanks you!

Enhance Bitbucket provider functionality and update secret configurat…

ca95e87

…ion template.

qodo-merge-for-open-source bot added the Review effort 2/5 label Apr 8, 2025

Prateikx reviewed Apr 9, 2025

View reviewed changes

pr_agent/git_providers/bitbucket_provider.py Outdated Show resolved Hide resolved

Prateikx reviewed Apr 9, 2025

View reviewed changes

pr_agent/git_providers/bitbucket_provider.py Outdated Show resolved Hide resolved

Update pr_agent/git_providers/bitbucket_provider.py

c0c3075

Co-authored-by: Prateek <110811408+Prateikx@users.noreply.github.com>

Update pr_agent/git_providers/bitbucket_provider.py

0cbf65d

Co-authored-by: Prateek <110811408+Prateikx@users.noreply.github.com>

Chunting Wu added 3 commits April 11, 2025 16:20

Extract repeated token retrieval logic into a helper function to redu…

5f2d4d4

…ce code duplication

Update doc for auth_type and basic_token

3c8ad9e

Fix Bearer backward compatibility logic

7a32faf

mrT23 approved these changes Apr 11, 2025

View reviewed changes

mrT23 merged commit a649e32 into qodo-ai:main Apr 11, 2025

wirelessr deleted the feat/issue-1681 branch April 14, 2025 01:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support Bitbucket Basic Auth #1683

Support Bitbucket Basic Auth #1683

Uh oh!

wirelessr commented Apr 8, 2025 •

edited by qodo-merge-for-open-source bot

Loading

Uh oh!

qodo-merge-for-open-source bot commented Apr 8, 2025 •

edited

Loading

Uh oh!

qodo-merge-for-open-source bot commented Apr 8, 2025 •

edited

Loading

Uh oh!

pythonclcoding commented Apr 9, 2025

Uh oh!

qodo-merge-for-open-source bot commented Apr 9, 2025

Uh oh!

pythonclcoding commented Apr 9, 2025

Uh oh!

qodo-merge-for-open-source bot commented Apr 9, 2025

Uh oh!

pythonclcoding commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

mrT23 commented Apr 11, 2025

Uh oh!

mrT23 commented Apr 11, 2025

Uh oh!

mrT23 commented Apr 11, 2025 •

edited

Loading

Uh oh!

wirelessr commented Apr 11, 2025

Uh oh!

federoitman commented Apr 11, 2025

Uh oh!

mrT23 commented Apr 12, 2025

Uh oh!

federoitman commented Apr 14, 2025

Uh oh!

Uh oh!

Support Bitbucket Basic Auth #1683

Support Bitbucket Basic Auth #1683

Uh oh!

Conversation

wirelessr commented Apr 8, 2025 • edited by qodo-merge-for-open-source bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-merge-for-open-source bot commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit ca95e87)

Uh oh!

qodo-merge-for-open-source bot commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

pythonclcoding commented Apr 9, 2025

Uh oh!

qodo-merge-for-open-source bot commented Apr 9, 2025

Title

User description

PR Type

Description

Changes walkthrough 📝

Uh oh!

pythonclcoding commented Apr 9, 2025

Uh oh!

qodo-merge-for-open-source bot commented Apr 9, 2025

Uh oh!

pythonclcoding commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

mrT23 commented Apr 11, 2025

Uh oh!

mrT23 commented Apr 11, 2025

Uh oh!

mrT23 commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wirelessr commented Apr 11, 2025

Uh oh!

federoitman commented Apr 11, 2025

Uh oh!

mrT23 commented Apr 12, 2025

Uh oh!

federoitman commented Apr 14, 2025

Uh oh!

Uh oh!

wirelessr commented Apr 8, 2025 •

edited by qodo-merge-for-open-source bot

Loading

qodo-merge-for-open-source bot commented Apr 8, 2025 •

edited

Loading

(Review updated until commit `ca95e87`)

qodo-merge-for-open-source bot commented Apr 8, 2025 •

edited

Loading

mrT23 commented Apr 11, 2025 •

edited

Loading