Fix GitLab /ask_line for single-line diff notes

[evanscastonguay]

User description

Summary

• Handle GitLab diff notes that do not include line_range (single-line notes)
• Correctly map LEFT/RIGHT side and file path for both old/new ranges
• Add unit tests for multi-line and single-line note payloads

Testing

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

---

PR Type

Bug fix, Tests

---

Description

• Handle GitLab single-line diff notes without line_range field

• Support both old and new file sides with correct path mapping

• Add comprehensive unit tests for multi-line and single-line scenarios

• Improve error handling with validation for missing line numbers/paths

---

Diagram Walkthrough

flowchart LR
  A["GitLab webhook payload"] -->|"with line_range"| B["Extract multi-line range"]
  A -->|"without line_range"| C["Extract single-line position"]
  B --> D["Determine side: LEFT/RIGHT"]
  C --> D
  D --> E["Map file path: old/new"]
  E --> F["Build /ask_line command"]
  G["Unit tests"] -->|"multi-line cases"| H["Validate range extraction"]
  G -->|"single-line cases"| I["Validate position extraction"]

Loading

File Walkthrough

	Relevant files
Bug fix	gitlab_webhook.py Enhanced /ask_line handler for single-line diff notes        pr_agent/servers/gitlab_webhook.py Refactored handle_ask_line() to handle both multi-line ranges and single-line notes Added support for extracting line numbers from position.new_line or position.old_line when line_range is absent Implemented proper LEFT/RIGHT side determination based on line type (old vs new) Added validation to ensure line numbers and file paths are present before processing Improved defensive coding with .get() methods and null checks +26/-10
Tests	test_gitlab_webhook.py Add unit tests for GitLab webhook ask_line handler              tests/unittest/test_gitlab_webhook.py Created new test file with parametrized tests for multi-line diff notes Added test cases for both "new" and "old" line types with correct side/path mapping Added parametrized tests for single-line diff notes without line_range Included pytest fixture to set required GITLAB__URL environment variable +71/-0

---

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

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Command injection risk Description: The body variable is constructed using unsanitized user input from question, path, and comment_id without validation or escaping, potentially enabling command injection if these values contain malicious shell metacharacters or command separators. gitlab_webhook.py [308-308] Referred Code body = f"/ask_line --line_start={start_line} --line_end={end_line} --side={side} --file_name={path} --comment_id={comment_id} {question}" except Exception as e:
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: Nested conditional structure: The function uses nested if-else structure for handling line_range presence instead of early return pattern for edge cases Referred Code if line_range_: range_type = line_range_.get('start', {}).get('type') side = 'RIGHT' if range_type == 'new' else 'LEFT' if range_type == 'new': start_line = line_range_.get('start', {}).get('new_line') end_line = line_range_.get('end', {}).get('new_line') path = position.get('new_path') else: start_line = line_range_.get('start', {}).get('old_line') end_line = line_range_.get('end', {}).get('old_line') path = position.get('old_path') else: start_line = position.get('new_line') or position.get('old_line') end_line = position.get('new_line') or position.get('old_line') side = 'RIGHT' if position.get('new_line') is not None else 'LEFT' path = position.get('new_path') or position.get('old_path')

Compliance status legend

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

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

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect file path selection Refactor the logic for single-line comments to correctly associate new_path with new_line and old_path with old_line, preventing potential file path mismatches. pr_agent/servers/gitlab_webhook.py [294-298] else: - start_line = position.get('new_line') or position.get('old_line') - end_line = position.get('new_line') or position.get('old_line') - side = 'RIGHT' if position.get('new_line') is not None else 'LEFT' - path = position.get('new_path') or position.get('old_path') + if position.get('new_line') is not None: + start_line = end_line = position.get('new_line') + side = 'RIGHT' + path = position.get('new_path') + else: + start_line = end_line = position.get('old_line') + side = 'LEFT' + path = position.get('old_path') Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a bug in the new logic where an incorrect file path could be used for comments on old lines, and the proposed fix is correct.	Medium
General	Improve test assertion for line numbers Strengthen the assertions in test_handle_ask_line_single_line to verify the exact values of --line_start and --line_end, not just their presence. tests/unittest/test_gitlab_webhook.py [62-71] def test_handle_ask_line_single_line(position, expected_side, expected_path): module = _load_module() payload = _base_payload(position) + line = position.get("new_line") or position.get("old_line") result = module.handle_ask_line("/ask explain", payload) - assert "--line_start" in result - assert "--line_end" in result + assert f"--line_start={line}" in result + assert f"--line_end={line}" in result assert f"--side={expected_side}" in result assert f"--file_name={expected_path}" in result Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that the test assertions are too weak and proposes a valid improvement to verify the exact line numbers, making the test more robust.	Low
Learned best practice	Validate range_type against allowed values The range_type should be validated against expected values ('new' or 'old') before use. Add explicit validation to catch unexpected values that could indicate data corruption or API changes. pr_agent/servers/gitlab_webhook.py [283-285] if line_range_: range_type = line_range_.get('start', {}).get('type') + if range_type not in ['new', 'old']: + raise ValueError(f"Invalid range_type: '{range_type}'. Must be 'new' or 'old'") side = 'RIGHT' if range_type == 'new' else 'LEFT' Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - When validating configuration values that should be from a restricted set of options, explicitly check the value against allowed options and raise a clear error for invalid values.	Low
	Add specific exception handling The exception is already captured correctly with 'as e' syntax. However, consider adding more specific exception types before the generic Exception catch to handle known error cases (like ValueError) separately for better error handling granularity. pr_agent/servers/gitlab_webhook.py [309-310] +except ValueError as e: + get_logger().error(f"Failed to handle ask line comment - validation error: {e}") except Exception as e: get_logger().error(f"Failed to handle ask line comment: {e}") Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - When catching exceptions in try-except blocks, always capture the exception object using 'as e' syntax to enable proper error logging and debugging.	Low
More

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