Enhanced gitlab webhook handling for push events without object_attributes

[GOOD21]

User description

fix push events don't automatically trigger pr_commands due to missing object_attributes

Push events don't have object_attributes, so it's impossible to filter based on the title or other information, meaning push operations shouldn't involve any execution logic checks.

Fortunately, for existing MRs, a push event will result in an update in the MR (push operations without an MR won't have a merge request event). Thus, we can determine if it's a merge request triggered by a push by checking if it includes object_attributes.oldrev.

This way, the logic related to push events can be removed.

---

PR Type

Bug fix, Enhancement

---

Description

• Fixed handling of push events without object_attributes.

• Enhanced logging for better debugging and event processing.

• Improved conditional checks for event types and commands.

---

Changes walkthrough 📝

	Relevant files
Enhancement	gitlab_webhook.py Improved event handling and logging in webhook                      pr_agent/servers/gitlab_webhook.py Added logging for missing object_attributes in event data. Enhanced conditional checks for push and note event types. Improved logic to handle push_commands and pr_commands. Refined logging messages for better debugging context. +5/-4

---

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-merge-pro-for-open-source]

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 Flow The condition for processing push events may need validation - the code checks for both event_type and object_kind which could potentially miss some valid push events if data.get('event_type') != 'note' and data.get('object_kind') != 'push': # not a comment Error Handling The should_process_pr_logic function returns False when object_attributes is missing, which may incorrectly reject valid push events that don't have this field if not data.get('object_attributes', {}): get_logger().info("No object attributes found in the data") return False

[qodo-merge-pro-for-open-source]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	✅ Optimize event type validation flow Suggestion Impact: The commit reorganizes the event handling logic, moving event type checks earlier in the flow. While not an exact match to the suggestion, it achieves similar optimization by handling different event types (merge_request, note) in separate conditional blocks code diff: - if data.get('event_type') != 'note' and data.get('object_kind') != 'push': # not a comment + + log_context["sender"] = sender + if data.get('object_kind') == 'merge_request': # ignore MRs based on title, labels, source and target branches - get_logger().info(f"Processing {data.get('object_kind')} event") if not should_process_pr_logic(data): return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"})) The condition for checking event types should be moved before the PR logic check to avoid unnecessary processing. Move the event type check to be right after the bot user check. pr_agent/servers/gitlab_webhook.py [191-198] # ignore bot users if is_bot_user(data): return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"})) -if data.get('event_type') != 'note' and data.get('object_kind') != 'push': # not a comment - # ignore MRs based on title, labels, source and target branches - get_logger().info(f"Processing {data.get('object_kind')} event") - if not should_process_pr_logic(data): - return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"})) +event_type = data.get('event_type') +object_kind = data.get('object_kind') +if event_type != 'note' and object_kind != 'push': + get_logger().info(f"Skipping non-note/push event: {object_kind}") + return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"})) + +# ignore MRs based on title, labels, source and target branches +if not should_process_pr_logic(data): + return JSONResponse(status_code=status.HTTP_200_OK, content=jsonable_encoder({"message": "success"})) + [Suggestion has been applied] Suggestion importance[1-10]: 7 __ Why: The suggestion improves code clarity and efficiency by reorganizing event type validation, preventing unnecessary processing, and adding more descriptive logging. The early return pattern for non-relevant events is a good practice.	Medium
	✅ Simplify command processing logic flow Suggestion Impact: The commit simplified the command processing logic by removing redundant conditions, though implemented differently than suggested. Instead of combining conditions, it removed one condition and kept the essential check. code diff: 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=}", **log_context) return - if commands_conf != "push_commands" and not should_process_pr_logic(data): - get_logger().info(f"Skipping auto commands for PR {api_url=}", **log_context) + if not should_process_pr_logic(data): # Here we already updated the configurations return The condition in _perform_commands_gitlab has redundant return statements and unclear logic flow. Combine the conditions for better readability and maintainability. pr_agent/servers/gitlab_webhook.py [63-68] -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=}", **log_context) - return -if commands_conf != "push_commands" and not should_process_pr_logic(data): - get_logger().info(f"Skipping auto commands for PR {api_url=}", **log_context) +should_skip = (commands_conf == "pr_commands" and get_settings().config.disable_auto_feedback) or \ + (commands_conf != "push_commands" and not should_process_pr_logic(data)) +if should_skip: + get_logger().info(f"Skipping auto commands for PR {api_url=}, reason: {'auto feedback disabled' if get_settings().config.disable_auto_feedback else 'PR logic check failed'}", **log_context) return [Suggestion has been applied] Suggestion importance[1-10]: 6 __ Why: The suggestion consolidates multiple conditions into a single, more readable check and improves the logging message by including the specific reason for skipping. This enhances code maintainability and debugging capabilities.	Low
Learned best practice	Add proper null safety checks when accessing dictionary values to prevent potential KeyError exceptions The code accesses nested dictionary keys without proper validation, which could lead to KeyError exceptions. Add proper null safety checks when accessing nested dictionary values, especially for the event type and object kind checks. pr_agent/servers/gitlab_webhook.py [194-196] -if data.get('event_type') != 'note' and data.get('object_kind') != 'push': # not a comment +event_type = data.get('event_type', '') +object_kind = data.get('object_kind', '') +if event_type != 'note' and object_kind != 'push': # not a comment # ignore MRs based on title, labels, source and target branches - get_logger().info(f"Processing {data.get('object_kind')} event") + get_logger().info(f"Processing {object_kind} event") Apply this suggestion Suggestion importance[1-10]: 6	Low
Update

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

[GOOD21]

Push events don't have object_attributes, so it's impossible to filter based on the title or other information, meaning push operations shouldn't involve any execution logic checks.

Fortunately, for existing MRs, a push event will result in an update in the MR (push operations without an MR won't have a merge request event). Thus, we can determine if it's a merge request triggered by a push by checking if it includes object_attributes.oldrev.

This way, the logic related to push events can be removed.

[mrT23]

@GOOD21
This is definitely simpler (and less API calls) than using 'get_mr_url_from_commit_sha'.
However, I am wondering if it is reliable, and if its specific only for push events.

Do you think that this 'oldrev' object will appear for all versions of gitlab ? do you have some documentation about it ?

[mrT23]

cc
@ofir-frd

[GOOD21]

@GOOD21 This is definitely simpler (and less API calls) than using 'get_mr_url_from_commit_sha'. However, I am wondering if it is reliable, and if its specific only for push events.

Do you think that this 'oldrev' object will appear for all versions of gitlab ? do you have some documentation about it ?

@mrT23 Yes, I checked the official GitLab documentation, and this field is present in at least versions 15, 16, and 17. It only has a value when there are actual code changes.

The field object_attributes.oldrev is only available when there are actual code changes, like:

• New code is pushed.
• A suggestion is applied.

[atrepczik]

LGTM

[DanaFineTLV]

@GOOD21 Thank you very much for your contribution! You are a Super Qoder! 🥇

We would like to thank you and send you SWAG. If you are ok can we get in contact? Does any of these communication channels work for you? Discord/Linkedin