Improvement: Enhance ask_line tool by adding PR review comment threads as context

[benedict-lee]

User description

Background.
When using the ask_line tool, conversations from existing author and reviewers are not included in the context.

Changes

• Implemented functionality to collect entire conversation history from code review threads
  • Applied hierarchical thread search algorithm that includes sibling comments and conversation threads
  • Maintained natural conversation flow through chronological sorting
• Enhanced context awareness by including conversation history in AI prompts
  • Enabled continuous conversations with preserved question-answer flow

PR Review thread example

Prompt example

---

PR Type

Enhancement, Tests

---

Description

• Added functionality to retrieve and process review thread comments.

• Enhanced AI prompts with conversation history for better context.

• Updated configuration to enable conversation history usage.

• Improved PR line question handling with review thread integration.

---

Changes walkthrough 📝

	Relevant files
Enhancement	git_provider.py Add methods for review comment and thread retrieval            pr_agent/git_providers/git_provider.py Added abstract methods for retrieving review comments and threads. Introduced methods to fetch review comment by ID and thread comments. Enhanced support for handling review threads in Git provider interface. +9/-0      github_provider.py Implement GitHub-specific review thread comment handling  pr_agent/git_providers/github_provider.py Implemented methods to fetch review comments and threads using PyGitHub. Added logic to retrieve ancestors, descendants, and sibling comments. Enabled chronological sorting of thread comments. Included error handling for comment retrieval failures. +124/-0  pr_line_questions.py Integrate conversation history into PR line questions        pr_agent/tools/pr_line_questions.py Added logic to load conversation history from review threads. Integrated conversation history into AI prompt variables. Enhanced error handling for conversation history loading. Introduced settings for enabling conversation history usage. +75/-0    pr_line_questions_prompts.toml Update AI prompts to include conversation history                pr_agent/settings/pr_line_questions_prompts.toml Updated AI prompt template to include conversation history. Added conditional block for displaying previous discussions. Enhanced prompt clarity with context from review threads. +9/-0
Configuration changes	configuration.toml Add configuration for conversation history usage                  pr_agent/settings/configuration.toml Added use_conversation_history setting under pr_questions. Enabled configuration for conversation history usage. +1/-0

---

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: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Performance Concern The implementation fetches all PR comments multiple times when processing thread comments. Consider caching the result of self.pr.get_comments() to avoid redundant API calls, especially for PRs with many comments. all_comments = list(self.pr.get_comments()) for comment in all_comments: if comment.id == comment_id: return comment return None except Exception as e: get_logger().warning(f"Failed to get review comment {comment_id}, error: {e}") return None def get_review_id_by_comment_id(self, comment_id: int): """ Finds the review ID that a comment belongs to based on its comment ID. Args: comment_id: Review comment ID Returns: Review ID or None (if not found) """ try: comment = self.get_review_comment_by_id(comment_id) if comment: return getattr(comment, 'pull_request_review_id', None) return None except Exception as e: get_logger().warning(f"Failed to get review ID for comment {comment_id}, error: {e}") return None def get_review_thread_comments(self, comment_id: int): """ Retrieves all comments in the thread that a specific comment belongs to. Args: comment_id: Review comment ID Returns: List of comments in the same thread """ try: # Get comment information comment = self.get_review_comment_by_id(comment_id) if not comment: return [] # get all comments all_comments = list(self.pr.get_comments()) Error Handling The conversation history loading function has nested try-except blocks that may mask specific errors. Consider refining the error handling to provide more granular error messages and better debugging information. try: comment_id = get_settings().get('comment_id', '') file_path = get_settings().get('file_name', '') line_number = get_settings().get('line_end', '') # return if no comment id or file path and line number if not (comment_id or (file_path and line_number)): return # initialize conversation history conversation_history = [] if hasattr(self.git_provider, 'get_review_thread_comments') and comment_id: try: # get review thread comments thread_comments = self.git_provider.get_review_thread_comments(comment_id) # current question id (this question is excluded from the context) current_question_id = comment_id # generate conversation history from the comments for comment in thread_comments: # skip empty comments body = getattr(comment, 'body', '') if not body or not body.strip(): continue # except for current question if current_question_id and str(comment.id) == str(current_question_id): continue # remove the AI command (/ask etc) from the beginning of the comment (optional) clean_body = body if clean_body.startswith('/'): clean_body = clean_body.split('\n', 1)[-1] if '\n' in clean_body else '' if not clean_body.strip(): continue # author info user = comment.user author = user.login if hasattr(user, 'login') else 'Unknown' # confirm if the author is the current user (AI vs user) is_ai = 'bot' in author.lower() or '[bot]' in author.lower() role = 'AI' if is_ai else 'User' # append to the conversation history conversation_history.append(f"{role} ({author}): {clean_body}") # transform the conversation history to a string if conversation_history: self.vars["conversation_history"] = "\n\n".join(conversation_history) get_logger().info(f"Loaded {len(conversation_history)} comments from the code review thread") else: self.vars["conversation_history"] = "" except Exception as e: get_logger().warning(f"Failed to get review thread comments: {e}") self.vars["conversation_history"] = "" except Exception as e: get_logger().error(f"Error loading conversation history: {e}") self.vars["conversation_history"] = ""

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

PR Code Suggestions ✨

Latest suggestions up to 9c06b6b

Category	Suggestion	Impact
Possible issue	Validate line number format The code checks if parameters are missing by using all(), but doesn't validate that line_number is a valid integer. Since line_number comes from get_settings().get('line_end', ''), it could be an empty string which would pass the check but cause errors when used for comparison. pr_agent/tools/pr_line_questions.py [111-113] # early return if any required parameter is missing -if not all([comment_id, file_path, line_number]): +if not comment_id or not file_path or not line_number: return +# ensure line_number is an integer +try: + line_number = int(line_number) +except (ValueError, TypeError): + get_logger().warning(f"Invalid line number: {line_number}") + return + Apply this suggestion Suggestion importance[1-10]: 9 __ Why: This suggestion identifies a critical issue where line_number might be an empty string that passes the existence check but would cause errors when used for comparison or conversion. The improved code properly validates and converts line_number to an integer, preventing runtime errors.	High
	Handle missing line number The code doesn't handle the case where line_number is None, which could happen if the comment doesn't have a "line" or "original_line" attribute. This would cause the filter to fail silently and potentially return incorrect results. pr_agent/git_providers/github_provider.py [452-459] # Get all comments all_comments = list(self.pr.get_comments()) # Filter comments on the same line of the same file -thread_comments = [ - c for c in all_comments - if c.path == file_path and (c.raw_data.get("line") == line_number or c.raw_data.get("original_line") == line_number) -] +thread_comments = [] +if file_path and line_number is not None: + thread_comments = [ + c for c in all_comments + if c.path == file_path and (c.raw_data.get("line") == line_number or c.raw_data.get("original_line") == line_number) + ] Apply this suggestion Suggestion importance[1-10]: 8 __ Why: The suggestion addresses a potential bug where line_number could be None, causing silent failures in the filtering logic. This is a valid concern as the code extracts line_number from comment.raw_data which might not contain the expected keys.	Medium
Possible issue	Avoid circular import dependency The code imports GithubProvider directly from github_provider module, but this creates a circular import issue since pr_line_questions.py is already imported in other modules. Use the more generic approach of checking provider type with isinstance() without the explicit import. pr_agent/tools/pr_line_questions.py [61-63] -# currently only supports GitHub provider -if get_settings().pr_questions.use_conversation_history and isinstance(self.git_provider, GithubProvider): +# check if conversation history is enabled and provider supports it +if get_settings().pr_questions.use_conversation_history and hasattr(self.git_provider, 'get_review_thread_comments'): self._load_conversation_history() Apply this suggestion Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential circular import issue. Replacing the explicit GithubProvider import with a more generic check using hasattr() is a better approach that avoids dependency problems while maintaining the same functionality.	Medium
	Fix incorrect return type The method signature indicates it returns list[dict] but it actually returns a list of comment objects, not dictionaries. This mismatch between the declared return type and actual return value could cause type errors when using the function. pr_agent/git_providers/github_provider.py [432-441] -def get_review_thread_comments(self, comment_id: int) -> list[dict]: +def get_review_thread_comments(self, comment_id: int) -> list: """ Retrieves all comments in the same line as the given comment. Args: comment_id: Review comment ID Returns: - List of comments on the same line + List of comment objects on the same line """ Apply this suggestion Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a type annotation mismatch. The method signature declares it returns list[dict] but actually returns a list of comment objects. This fix improves type safety and prevents potential confusion or errors for users of this API.	Medium
Update

---

Previous suggestions

✅ Suggestions up to commit b53d277

Category	Suggestion	Impact
Possible issue	Prevent infinite recursion The recursive find_descendants function can cause a stack overflow for deeply nested comment threads. It's missing a check for circular references in the reply chain, which could lead to infinite recursion. pr_agent/git_providers/github_provider.py [503-511] # Recursively find all descendant comments (collect all replies to the comment) -def find_descendants(cid): +def find_descendants(cid, visited=None): + if visited is None: + visited = set() + if cid in visited: + return [] # Prevent infinite recursion + visited.add(cid) descendants = [] for c in all_comments: if getattr(c, 'in_reply_to_id', None) == cid: descendants.append(c.id) - descendants.extend(find_descendants(c.id)) + descendants.extend(find_descendants(c.id, visited)) return descendants Suggestion importance[1-10]: 9 __ Why: This suggestion addresses a critical issue that could cause stack overflow errors in production. Adding a visited set to track already processed comments prevents infinite recursion in case of circular references in the comment chain, which is a significant improvement for stability.	High
	Fix AI author identification The current method to identify AI comments is too simplistic and may misclassify users with 'bot' in their username as AI. This could lead to incorrect conversation history formatting and confuse the AI model about who said what. pr_agent/tools/pr_line_questions.py [149-151] # confirm if the author is the current user (AI vs user) -is_ai = 'bot' in author.lower() or '[bot]' in author.lower() +# Get the bot username from settings or use a more robust identification method +bot_username = getattr(get_settings(), 'bot_username', None) +is_ai = (bot_username and author == bot_username) or '[bot]' in author role = 'AI' if is_ai else 'User' Suggestion importance[1-10]: 7 __ Why: The suggestion improves the accuracy of identifying AI comments by using a more robust method that relies on configuration rather than simple string matching. This prevents misclassification of users with 'bot' in their username and improves the quality of conversation history.	Medium
	✅ Fix ID comparison logic Suggestion Impact: The commit addressed the same issue as the suggestion but implemented a different solution. Instead of converting both IDs to strings, the commit directly compares the IDs without any type conversion, assuming they are already of the same type. code diff: - if current_question_id and str(comment.id) == str(current_question_id): - continue + # except for current question + if current_question_id and comment.id == current_question_id: The code compares string and integer IDs without proper type conversion, which could lead to missed matches. The comparison str(comment.id) == str(current_question_id) assumes both values can be converted to strings, but if current_question_id is None or not a valid ID format, this could cause errors. pr_agent/tools/pr_line_questions.py [123-135] # current question id (this question is excluded from the context) current_question_id = comment_id # generate conversation history from the comments for comment in thread_comments: # skip empty comments body = getattr(comment, 'body', '') if not body or not body.strip(): continue # except for current question - if current_question_id and str(comment.id) == str(current_question_id): + if current_question_id and current_question_id.isdigit() and int(current_question_id) == comment.id: continue [Suggestion has been applied] Suggestion importance[1-10]: 6 __ Why: The suggestion improves the robustness of the ID comparison by properly handling type conversion and validation. This prevents potential errors when comparing comment IDs of different types and ensures correct filtering of the current question from conversation history.	Low
Learned best practice	Add null safety checks when traversing parent-child relationships to prevent potential runtime errors The current implementation assumes that in_reply_to_map always contains valid entries and doesn't check if parent_id is None before using it as the new current value. This could lead to runtime errors if the data structure is inconsistent. Add a null check to ensure parent_id is not None before continuing the loop. pr_agent/git_providers/github_provider.py [502-510] # Recursively find all ancestor comments (collect comment's ancestors) def find_ancestors(cid): ancestors = [] current = cid while current in in_reply_to_map: parent_id = in_reply_to_map[current] + if parent_id is None: + break ancestors.append(parent_id) current = parent_id return ancestors Suggestion importance[1-10]: 6	Low

[ofir-frd]

@benedict-lee ,

Thank you for this significant contribution.
I've made a couple of review comments on the code changes.
While reviewing, I noticed that the current version contains an error that prevents it from running. Please take a look at it.

[benedict-lee]

@ofir-frd
I apologize for sending a PR with embarrassing code at midnight yesterday.
In truth, I quickly implemented it using vibecoding and sent the PR without sufficient in-depth review. I've realized once again that even when using vibecoding, we must thoroughly examine AI-generated code.
There was absolutely no other intention behind it, so I hope there's no misunderstanding.

Since I haven't been working with Python for long (PR agent was my first introduction to Python), my refactored code might still be lacking.
I'm sorry, but please check it again and provide feedback.

[ofir-frd]

@benedict-lee
Thank you for your contribution.

The code logic was improved significantly and it now works.
I do have a couple more comments to reduce API calls, code complexity and readability.
Please have a look.

[ofir-frd]

Please make this more informative by describing the conversation structure and how the model should handle it.

[ofir-frd]

Please place this import in order with the others, next to the from pr_agent.git_providers... imports.

[ofir-frd]

This function currently introduces unnecessary side effects, making the code harder to read and follow. Let's build the conversation string inside the function and return it, so the run function can handle its output.

Suggested change

	def _load_conversation_history(self):
	def _load_conversation_history(self) -> str:

[ofir-frd]

Let's return an empty string in case the code crashes.

[ofir-frd]

Here, you're collecting all PR comments. You can expect to find the given comment_id within this list, making the comment = self.pr.get_comment(comment_id) code line redundant and reducing the number of API calls by half, from 2 to 1.

[ofir-frd]

Lets log this

[ofir-frd]

This is too general; let's work with comment IDs instead of non-unique attributes.
Hint: look for in_reply_to_id

[ofir-frd]

Please improve the sense of order in this string, for example, count the comments.

[benedict-lee]

@ofir-frd Thank you for your kind review! I've implemented all the refactoring changes you suggested.
Throughout the process of improving the code logic, I learned a lot about minimizing API calls, eliminating side effects, and applying consistent code patterns.
Thanks to your review, the code quality and readability have been significantly enhanced, and I'll continue to apply these patterns in the future. Really appreciate your valuable feedback!

[ofir-frd]

@benedict-lee

Looking good!
Last short round of comments.

[ofir-frd]

Thanks @benedict-lee !
Code approved!