Bitbucket: Add push_commands support when PR is updated #2085

sharoneyal · 2025-10-28T19:13:21Z

PR Type

Enhancement

Description

Add support for push_commands when Bitbucket PR is updated
Implement validation to ensure push commands only trigger from actual commits
Validate time delta between last commit and PR update (max 15 seconds)
Verify commit author matches PR updater to prevent false triggers

Diagram Walkthrough

flowchart LR
  A["Bitbucket pullrequest:updated event"] --> B["Validate push trigger enabled"]
  B --> C["Check time delta between commit and PR update"]
  C --> D["Verify commit author matches PR updater"]
  D --> E["Execute push_commands if valid"]
  D --> F["Skip if validation fails"]

File Walkthrough

Relevant files

Enhancement

bitbucket_app.py `Add push_commands validation and pullrequest:updated handler` pr_agent/servers/bitbucket_app.py Added `_validate_time_from_last_commit_to_pr_update()` function to validate push events by checking time delta and author match Added handler for `pullrequest:updated` event to trigger `push_commands` with validation Added configuration check for `bitbucket_app.handle_push_trigger` setting Fixed agent instantiation to reuse existing agent instead of creating new `PRAgent()` instance	+76/-1

qodo-merge-for-open-source · 2025-10-28T19:13:52Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
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
	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
⚪	Robust Error Handling Objective: Ensure potential errors and edge cases are anticipated and handled gracefully throughout the code Status: Time parsing robustness: The new validation parses ISO timestamps with datetime.fromisoformat without handling timezone/Z suffix variants or non-ISO responses, which may raise exceptions or miscompute diffs depending on Bitbucket formats. Referred Code from datetime import datetime ts1 = datetime.fromisoformat(time_pr_updated) ts2 = datetime.fromisoformat(time_last_commit) diff = (ts1 - ts2).total_seconds() max_delta_seconds = 15 if diff > 0 and diff < max_delta_seconds: is_valid_push = True else: get_logger().debug(f"Too much time passed since last commit", artifact={'updated': time_pr_updated, 'last_commit': time_last_commit})
	Single Responsibility for Functions Objective: Each function should have a single, well-defined responsibility Status: Mixed concerns: _perform_commands_bitbucket handles config checks, event gating, validation, command iteration, and logging, which may span multiple responsibilities. Referred Code async def _perform_commands_bitbucket(commands_conf: str, agent: PRAgent, api_url: str, log_context: dict, data: dict): apply_repo_settings(api_url) if commands_conf == "pr_commands" and get_settings().config.disable_auto_feedback: # auto commands for PR, and auto feedback is disabled get_logger().info(f"Auto feedback is disabled, skipping auto commands for PR {api_url=}") return if commands_conf == "push_commands": if not get_settings().get("bitbucket_app.handle_push_trigger"): get_logger().info( "Bitbucket push trigger handling disabled via 'bitbucket_app.handle_push_trigger'; skipping push commands") return if data.get("event", "") == "pullrequest:created": if not should_process_pr_logic(data): return commands = get_settings().get(f"bitbucket_app.{commands_conf}", {}) get_settings().set("config.is_auto_command", True) if commands_conf == "push_commands": is_valid_push = await _validate_time_from_last_commit_to_pr_update(data) if not is_valid_push: get_logger().info(f"Bitbucket skipping 'pullrequest:updated' for push commands") return for command in commands:
Update

Compliance status legend

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

qodo-merge-for-open-source · 2025-10-28T19:15:29Z

PR Code Suggestions ✨

Latest suggestions up to 5886bae

Category	Suggestion	Impact
Possible issue	Use stable IDs for author match Use the stable `account_id` for matching the webhook actor with the commit author instead of the non-unique `display_name`. Fall back to other usernames if `account_id` is unavailable. pr_agent/servers/bitbucket_app.py [109-120] -username =_get_username(data) +username = _get_username(data) +actor_account_id = data.get('data', {}).get('actor', {}).get('account_id') + commits_data = response.json() or {} values = commits_data.get('values') or [] -if (not values or not isinstance(values, list) or not values[0].get('author') or not values[0]['author'].get('user') - or not values[0]['author']['user'].get('display_name')): - get_logger().warning("No commits returned for pull request or one of the required fields missing; skipping push validation", +if not values or not isinstance(values, list): + get_logger().warning("No commits returned for pull request; skipping push validation", artifact={'values': values}) return False -commit_username = commits_data['values'][0]['author']['user']['display_name'] -if username != commit_username: - get_logger().warning(f"Mismatch in username {username} vs. commit_username {commit_username}") - return False +latest_commit = values[0] +commit_user = (latest_commit.get('author') or {}).get('user') or {} +commit_account_id = commit_user.get('account_id') +commit_username = commit_user.get('nickname') or commit_user.get('username') or commit_user.get('display_name') + +# Prefer stable account_id comparison when available +if actor_account_id and commit_account_id: + if actor_account_id != commit_account_id: + get_logger().warning("Mismatch in actor and commit author account_id; skipping push validation", + artifact={'actor_account_id': actor_account_id, 'commit_account_id': commit_account_id}) + return False +else: + # Fallback to less-stable username comparisons + if not commit_username or username != commit_username: + get_logger().warning("Mismatch in actor and commit author username; skipping push validation", + artifact={'actor_username': username, 'commit_username': commit_username}) + return False + Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out that `display_name` is not a reliable identifier for user matching. Using a stable unique identifier like `account_id` significantly improves the accuracy and reliability of the user validation logic.	Medium
Possible issue	Normalize timestamps to UTC Normalize timestamps to UTC before calculating the difference to prevent errors from mixed timezone-aware and naive datetimes. pr_agent/servers/bitbucket_app.py [125-133] -ts1 = datetime.fromisoformat(time_pr_updated) -ts2 = datetime.fromisoformat(time_last_commit) -diff = (ts1 - ts2).total_seconds() +from datetime import datetime, timezone + +def _to_utc(dt: datetime) -> datetime: + return dt if dt.tzinfo else dt.replace(tzinfo=timezone.utc) + +ts1 = _to_utc(datetime.fromisoformat(time_pr_updated)) +ts2 = _to_utc(datetime.fromisoformat(time_last_commit)) +diff = (ts1.astimezone(timezone.utc) - ts2.astimezone(timezone.utc)).total_seconds() max_delta_seconds = 15 -if diff > 0 and diff < max_delta_seconds: +if 0 < diff < max_delta_seconds: is_valid_push = True else: - get_logger().debug(f"Too much time passed since last commit", - artifact={'updated': time_pr_updated, 'last_commit': time_last_commit}) + get_logger().debug("Too much time passed since last commit", + artifact={'updated': time_pr_updated, 'last_commit': time_last_commit, 'diff_seconds': diff}) Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential issue with comparing timezone-aware and timezone-naive datetimes, and the proposed change improves the code's robustness by explicitly handling timezones, which is a good practice.	Low
Incremental ^[*]	Reset auto-command flag on exit Reset the `config.is_auto_command` flag to `False` before exiting early from push command handling to prevent potential side effects from an incorrect flag state. pr_agent/servers/bitbucket_app.py [144-158] if commands_conf == "push_commands": if not get_settings().get("bitbucket_app.handle_push_trigger"): - get_logger().info( - "Bitbucket push trigger handling disabled via config; skipping push commands") + get_logger().info("Bitbucket push trigger handling disabled via config; skipping push commands") + get_settings().set("config.is_auto_command", False) return ... if commands_conf == "push_commands": is_valid_push = await _validate_time_from_last_commit_to_pr_update(data) if not is_valid_push: - get_logger().info(f"Bitbucket skipping 'pullrequest:updated' for push commands") + get_logger().info("Bitbucket skipping 'pullrequest:updated' for push commands") + get_settings().set("config.is_auto_command", False) return `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that an early return leaves the `config.is_auto_command` flag in an incorrect state, which could cause unintended side effects. Resetting the flag improves the robustness of the logic.	Medium
Learned best practice	Safely access nested config flags Use a safe accessor for nested settings and provide an explicit boolean default to prevent None/KeyError issues in configuration. pr_agent/servers/bitbucket_app.py [145-148] -if not get_settings().get("bitbucket_app.handle_push_trigger"): - get_logger().info( - "Bitbucket push trigger handling disabled via config; skipping push commands") +if not get_settings().get("bitbucket_app.handle_push_trigger", False): + get_logger().info("Bitbucket push trigger handling disabled via config; skipping push commands") return Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Guard nested config/attributes with safe accessors to avoid KeyError/None usage and provide sensible defaults.	Low
More

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

Previous suggestions

Suggestions up to commit 46c47e1

Category	Suggestion	Impact
High-level	Use an async HTTP client The `async` function `_validate_time_from_last_commit_to_pr_update` uses the synchronous `requests` library for an API call, which blocks the event loop. This should be replaced with an asynchronous HTTP client like `httpx` to avoid performance bottlenecks. Examples: pr_agent/servers/bitbucket_app.py [86-138] async def _validate_time_from_last_commit_to_pr_update(data: dict) -> bool: is_valid_push = False try: data_inner = data.get('data', {}) if not data_inner: get_logger().error("No data found in the webhook payload") return True pull_request = data_inner.get('pullrequest', {}) commits_api = pull_request.get('links', {}).get('commits', {}).get('href') if not commits_api: ... (clipped 43 lines) Solution Walkthrough: Before: async def _validate_time_from_last_commit_to_pr_update(data: dict) -> bool: try: # ... commits_api = pull_request.get('links', {}).get('commits', {}).get('href') # ... headers = { 'Authorization': f'Bearer {bearer_token}', 'Accept': 'application/json' } # Synchronous call blocking the event loop response = requests.get(commits_api, headers=headers) # ... except Exception as e: # ... return is_valid_push After: import httpx # or aiohttp async def _validate_time_from_last_commit_to_pr_update(data: dict) -> bool: try: # ... commits_api = pull_request.get('links', {}).get('commits', {}).get('href') # ... headers = { 'Authorization': f'Bearer {bearer_token}', 'Accept': 'application/json' } # Asynchronous call that doesn't block async with httpx.AsyncClient() as client: response = await client.get(commits_api, headers=headers) # ... except Exception as e: # ... return is_valid_push Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a blocking `requests.get()` call within an `async` function, which is a critical performance issue that undermines the async architecture of the server.	High
Possible issue	Avoid blocking the event loop In the `_validate_time_from_last_commit_to_pr_update` async function, wrap the synchronous `requests.get()` call with `asyncio.get_event_loop().run_in_executor()` to prevent blocking the event loop. pr_agent/servers/bitbucket_app.py [104] -response = requests.get(commits_api, headers=headers) +import asyncio +... +response = await asyncio.get_event_loop().run_in_executor( + None, lambda: requests.get(commits_api, headers=headers, timeout=30) +) Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a blocking I/O call (`requests.get`) inside an `async` function, which can degrade server performance by blocking the event loop. The proposed fix is appropriate.	Medium
General	Simplify commit data extraction logic Refactor the commit data extraction logic by first storing the latest commit in a variable and then safely accessing its nested properties using `.get()`. This will simplify the code, reduce repetition, and improve readability. pr_agent/servers/bitbucket_app.py [112-123] -if (not values or not isinstance(values, list) or not values[0].get('author') or not values[0]['author'].get('user') - or not values[0]['author']['user'].get('display_name')): - get_logger().warning("No commits returned for pull request or one of the required fields missing; skipping push validation", +values = commits_data.get('values') or [] +if not values or not isinstance(values, list): + get_logger().warning("No commits returned for pull request; skipping push validation", artifact={'values': values}) return False -commit_username = commits_data['values'][0]['author']['user']['display_name'] + +latest_commit = values[0] +commit_username = latest_commit.get('author', {}).get('user', {}).get('display_name') +time_last_commit = latest_commit.get('date') + +if not commit_username or not time_last_commit: + get_logger().warning("Latest commit is missing required fields (author's display_name or date); skipping push validation", + artifact={'commit': latest_commit}) + return False + if username != commit_username: get_logger().warning(f"Mismatch in username {username} vs. commit_username {commit_username}") return False time_pr_updated = pull_request['updated_on'] -time_last_commit = commits_data['values'][0]['date'] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies repetitive and complex dictionary access. The proposed refactoring improves code readability and maintainability by reducing repetition and using safer access patterns.	Low
General	Make the time delta configurable Replace the hardcoded `max_delta_seconds` value with a configurable setting retrieved via `get_settings()` to improve maintainability. pr_agent/servers/bitbucket_app.py [128] -max_delta_seconds = 15 +max_delta_seconds = get_settings().get("bitbucket_app.push_trigger_max_commit_time_delta", 15) Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out a hardcoded value and proposes making it configurable, which is a good practice for maintainability and flexibility.	Low
Learned best practice	Safely access bearer token Safely access `bitbucket_bearer_token` and validate it before use to avoid sending an invalid Authorization header. Log a clear warning and abort validation when missing. pr_agent/servers/bitbucket_app.py [99-104] -bearer_token = context.get('bitbucket_bearer_token') +bearer_token = context.get('bitbucket_bearer_token', None) +if not bearer_token: + get_logger().warning("Missing Bitbucket bearer token; skipping push validation") + return False headers = { 'Authorization': f'Bearer {bearer_token}', 'Accept': 'application/json' } response = requests.get(commits_api, headers=headers) Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Guard access to nested settings and attributes with safe accessors and validation, providing defaults or clear errors when missing.	Low
Learned best practice	Robust timestamp parsing Parse ISO timestamps defensively to handle timezone offsets/Z suffix and validate types before computing the diff; fall back gracefully on parsing errors. pr_agent/servers/bitbucket_app.py [122-130] -time_pr_updated = pull_request['updated_on'] -time_last_commit = commits_data['values'][0]['date'] -from datetime import datetime -ts1 = datetime.fromisoformat(time_pr_updated) -ts2 = datetime.fromisoformat(time_last_commit) +time_pr_updated = pull_request.get('updated_on') +time_last_commit = commits_data.get('values', [{}])[0].get('date') +from datetime import datetime, timezone +def _parse_iso(ts: str): + if not isinstance(ts, str): return None + ts = ts.strip().replace('Z', '+00:00') + try: + return datetime.fromisoformat(ts) + except Exception: + return None +ts1 = _parse_iso(time_pr_updated) +ts2 = _parse_iso(time_last_commit) +if not ts1 or not ts2: + get_logger().warning("Failed to parse timestamps for push validation", + artifact={'updated': time_pr_updated, 'last_commit': time_last_commit}) + return False diff = (ts1 - ts2).total_seconds() max_delta_seconds = 15 -if diff > 0 and diff < max_delta_seconds: +if 0 < diff < max_delta_seconds: is_valid_push = True Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Replace brittle string operations and ad-hoc logic with robust parsing and validation for external inputs.	Low

pr_agent/servers/bitbucket_app.py

Add push_commands support when PR is updated

46c47e1

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

ofir-frd reviewed Oct 29, 2025

View reviewed changes

pr_agent/servers/bitbucket_app.py Outdated Show resolved Hide resolved

ofir-frd reviewed Oct 29, 2025

View reviewed changes

pr_agent/servers/bitbucket_app.py Outdated Show resolved Hide resolved

code review

5886bae

sharoneyal merged commit 8bd134e into main Oct 29, 2025
2 checks passed

sharoneyal deleted the es/bitbucket_support_push_commands branch October 29, 2025 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bitbucket: Add push_commands support when PR is updated #2085

Bitbucket: Add push_commands support when PR is updated #2085

sharoneyal commented Oct 28, 2025 •

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

Loading

Uh oh!

qodo-merge-for-open-source bot commented Oct 28, 2025 •

edited

Loading

Uh oh!

qodo-merge-for-open-source bot commented Oct 28, 2025 •

edited

Loading

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Bitbucket: Add push_commands support when PR is updated #2085

Bitbucket: Add push_commands support when PR is updated #2085

Conversation

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

Uh oh!

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

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

Uh oh!

PR Compliance Guide 🔍

Uh oh!

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

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sharoneyal commented Oct 28, 2025 •

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

Loading

qodo-merge-for-open-source bot commented Oct 28, 2025 •

edited

Loading

qodo-merge-for-open-source bot commented Oct 28, 2025 •

edited

Loading