From: Simon Glass <simon.glass@canonical.com> Check GitLab merge request status to detect when a rebase is needed, even if there are no review comments. This handles the case where GitLab shows "Merge request must be rebased" or "Merge conflicts must be resolved". Add has_conflicts and needs_rebase fields to PickmanMr namedtuple, populated from GitLab's detailed_merge_status and diverged_commits_count fields. Update process_mr_reviews() to trigger the agent for rebase when needed, even without review comments. The agent prompt is adjusted based on whether it needs to handle comments, rebase, or both. Also: - Pass MR description to agent for context from previous work - After processing reviews, restore the original branch - Use -o ci.skip when pushing to avoid duplicate pipelines Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- (no changes since v1) tools/pickman/agent.py | 89 ++++++++++++++++++++++++++++--------- tools/pickman/control.py | 34 +++++++++++--- tools/pickman/ftest.py | 63 ++++++++++++++++++++++++++ tools/pickman/gitlab_api.py | 16 ++++++- 4 files changed, 174 insertions(+), 28 deletions(-) diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py index bb922b03b13..14aab64a6dd 100644 --- a/tools/pickman/agent.py +++ b/tools/pickman/agent.py @@ -137,8 +137,9 @@ def cherry_pick_commits(commits, source, branch_name, repo_path=None): async def run_review_agent(mr_iid, branch_name, comments, remote, target='master', - repo_path=None): - """Run the Claude agent to handle MR comments + needs_rebase=False, has_conflicts=False, + mr_description='', repo_path=None): + """Run the Claude agent to handle MR comments and/or rebase Args: mr_iid (int): Merge request IID @@ -146,6 +147,9 @@ async def run_review_agent(mr_iid, branch_name, comments, remote, target='master comments (list): List of comment dicts with 'author', 'body' keys remote (str): Git remote name target (str): Target branch for rebase operations + needs_rebase (bool): Whether the MR needs rebasing + has_conflicts (bool): Whether the MR has merge conflicts + mr_description (str): MR description with context from previous work repo_path (str): Path to repository (defaults to current directory) Returns: @@ -157,35 +161,75 @@ async def run_review_agent(mr_iid, branch_name, comments, remote, target='master if repo_path is None: repo_path = os.getcwd() - # Format comments for the prompt - comment_text = '\n'.join( - f'- [{c.author}]: {c.body}' - for c in comments - ) + # Build the prompt based on what needs to be done + tasks = [] + if needs_rebase: + if has_conflicts: + tasks.append("rebase and resolve merge conflicts") + else: + tasks.append("rebase onto latest target branch") + if comments: + tasks.append(f"address {len(comments)} review comment(s)") + + task_desc = " and ".join(tasks) + + # Include MR description for context from previous work + context_section = "" + if mr_description: + context_section = f""" +Context from MR description (includes previous work done on this MR): - prompt = f"""Review comments on merge request !{mr_iid} (branch: {branch_name}): +{mr_description} + +Use this context to understand what was done previously and respond appropriately. +""" + + # Format comments for the prompt (if any) + comment_section = "" + if comments: + comment_text = '\n'.join( + f'- [{c.author}]: {c.body}' + for c in comments + ) + comment_section = f""" +Review comments to address: {comment_text} +""" + + # Build rebase instructions + rebase_section = "" + if needs_rebase: + rebase_section = f""" +Rebase instructions: +- The MR is behind the target branch and needs rebasing +- Use: git rebase --keep-empty {remote}/{target} +- This preserves empty merge commits which are important for tracking +- If there are conflicts, try to resolve them automatically +- For complex conflicts that cannot be resolved, describe them and abort +""" + prompt = f"""Task for merge request !{mr_iid} (branch: {branch_name}): {task_desc} +{context_section}{comment_section}{rebase_section} Steps to follow: 1. Checkout the branch: git checkout {branch_name} -2. Read and understand each comment -3. For each actionable comment: - - Make the requested changes to the code - - Amend the relevant commit or create a fixup commit +2. {'Rebase onto ' + remote + '/' + target + ' first' if needs_rebase else 'Read and understand each comment'} +3. {'After rebase, address any review comments' if needs_rebase and comments else 'For each actionable comment:' if comments else 'Verify the rebase completed successfully'} + {'- Make the requested changes to the code' if comments else ''} + {'- Amend the relevant commit or create a fixup commit' if comments else ''} 4. Run 'crosfw sandbox -L' to verify the build 5. Create a local branch with suffix '-v2' (or increment: -v3, -v4, etc.) 6. Force push to the ORIGINAL remote branch to update the MR: - git push --force-with-lease {remote} HEAD:{branch_name} -7. Report what changes were made and what reply should be posted to the MR + git push -o ci.skip --force-with-lease {remote} HEAD:{branch_name} + (ci.skip prevents a duplicate pipeline; the MR pipeline will run automatically) +7. Report what was done and what reply should be posted to the MR Important: -- Keep changes minimal and focused on addressing the comments +- Keep changes minimal and focused - If a comment is unclear or cannot be addressed, note this in your report - Local branch: {branch_name}-v2 (or -v3, -v4 etc.) - Remote push: always to '{branch_name}' to update the existing MR -- If rebasing is requested, use: git rebase --keep-empty {remote}/{target} - This preserves empty merge commits which are important for tracking +- Always use -o ci.skip when pushing to avoid duplicate pipelines """ options = ClaudeAgentOptions( @@ -193,7 +237,7 @@ Important: cwd=repo_path, ) - tout.info(f'Starting Claude agent to handle {len(comments)} comment(s)...') + tout.info(f'Starting Claude agent to {task_desc}...') tout.info('') conversation_log = [] @@ -211,7 +255,8 @@ Important: def handle_mr_comments(mr_iid, branch_name, comments, remote, target='master', - repo_path=None): + needs_rebase=False, has_conflicts=False, + mr_description='', repo_path=None): """Synchronous wrapper for running the review agent Args: @@ -220,6 +265,9 @@ def handle_mr_comments(mr_iid, branch_name, comments, remote, target='master', comments (list): List of comment dicts remote (str): Git remote name target (str): Target branch for rebase operations + needs_rebase (bool): Whether the MR needs rebasing + has_conflicts (bool): Whether the MR has merge conflicts + mr_description (str): MR description with context from previous work repo_path (str): Path to repository (defaults to current directory) Returns: @@ -227,4 +275,5 @@ def handle_mr_comments(mr_iid, branch_name, comments, remote, target='master', conversation_log is the agent's output text """ return asyncio.run(run_review_agent(mr_iid, branch_name, comments, remote, - target, repo_path)) + target, needs_rebase, has_conflicts, + mr_description, repo_path)) diff --git a/tools/pickman/control.py b/tools/pickman/control.py index dec2c68883f..a0b79d9203c 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -663,6 +663,9 @@ def process_mr_reviews(remote, mrs, dbs, target='master'): Returns: int: Number of MRs with comments processed """ + # Save current branch to restore later + original_branch = run_git(['rev-parse', '--abbrev-ref', 'HEAD']) + # Fetch to get latest remote state (needed for rebase) tout.info(f'Fetching {remote}...') run_git(['fetch', remote]) @@ -673,7 +676,7 @@ def process_mr_reviews(remote, mrs, dbs, target='master'): mr_iid = merge_req.iid comments = gitlab_api.get_mr_comments(remote, mr_iid) if comments is None: - continue + comments = [] # Filter to unresolved comments that haven't been processed unresolved = [] @@ -683,21 +686,35 @@ def process_mr_reviews(remote, mrs, dbs, target='master'): if dbs.comment_is_processed(mr_iid, com.id): continue unresolved.append(com) - if not unresolved: + + # Check if rebase is needed + needs_rebase = merge_req.needs_rebase or merge_req.has_conflicts + + # Skip if no comments and no rebase needed + if not unresolved and not needs_rebase: continue tout.info('') - tout.info(f"MR !{mr_iid} has {len(unresolved)} new comment(s):") - for comment in unresolved: - tout.info(f' [{comment.author}]: {comment.body[:80]}...') + if needs_rebase: + if merge_req.has_conflicts: + tout.info(f"MR !{mr_iid} has merge conflicts - rebasing...") + else: + tout.info(f"MR !{mr_iid} needs rebase...") + if unresolved: + tout.info(f"MR !{mr_iid} has {len(unresolved)} new comment(s):") + for comment in unresolved: + tout.info(f' [{comment.author}]: {comment.body[:80]}...') - # Run agent to handle comments + # Run agent to handle comments and/or rebase success, conversation_log = agent.handle_mr_comments( mr_iid, merge_req.source_branch, unresolved, remote, target, + needs_rebase=needs_rebase, + has_conflicts=merge_req.has_conflicts, + mr_description=merge_req.description, ) if success: @@ -726,6 +743,11 @@ def process_mr_reviews(remote, mrs, dbs, target='master'): tout.error(f"Failed to handle comments for MR !{mr_iid}") processed += 1 + # Restore original branch + if processed: + tout.info(f'Returning to {original_branch}') + run_git(['checkout', original_branch]) + return processed diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 9d7786541c3..758733ce7f2 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -1917,6 +1917,69 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): dbs.close() + def test_rebase_without_comments(self): + """Test that MRs needing rebase trigger agent even without comments.""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + + # MR needs rebase but has no comments + mrs = [gitlab_api.PickmanMr( + iid=100, + title='[pickman] Test MR', + source_branch='cherry-test', + description='Test', + web_url='https://gitlab.com/mr/100', + has_conflicts=False, + needs_rebase=True, + )] + + with mock.patch.object(control, 'run_git'): + with mock.patch.object(gitlab_api, 'get_mr_comments', + return_value=[]): + with mock.patch.object(agent, 'handle_mr_comments', + return_value=(True, 'Rebased')) as mock_agent: + with mock.patch.object(gitlab_api, 'update_mr_description'): + with mock.patch.object(control, 'update_history_with_review'): + control.process_mr_reviews('ci', mrs, dbs) + + # Agent should be called with needs_rebase=True + mock_agent.assert_called_once() + call_kwargs = mock_agent.call_args[1] + self.assertTrue(call_kwargs.get('needs_rebase')) + self.assertFalse(call_kwargs.get('has_conflicts')) + + dbs.close() + + def test_skips_mr_no_rebase_no_comments(self): + """Test that MRs without rebase need or comments are skipped.""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + + # MR has no comments and doesn't need rebase + mrs = [gitlab_api.PickmanMr( + iid=100, + title='[pickman] Test MR', + source_branch='cherry-test', + description='Test', + web_url='https://gitlab.com/mr/100', + has_conflicts=False, + needs_rebase=False, + )] + + with mock.patch.object(control, 'run_git'): + with mock.patch.object(gitlab_api, 'get_mr_comments', + return_value=[]): + with mock.patch.object(agent, 'handle_mr_comments', + return_value=(True, 'Done')) as mock_agent: + control.process_mr_reviews('ci', mrs, dbs) + + # Agent should NOT be called + mock_agent.assert_not_called() + + dbs.close() + class TestParsePoll(unittest.TestCase): """Tests for poll command argument parsing.""" diff --git a/tools/pickman/gitlab_api.py b/tools/pickman/gitlab_api.py index 50c3ec67909..a4f4bce7cf2 100644 --- a/tools/pickman/gitlab_api.py +++ b/tools/pickman/gitlab_api.py @@ -28,9 +28,11 @@ except ImportError: # Merge request info returned by get_pickman_mrs() +# Use defaults for new fields so existing code doesn't break PickmanMr = namedtuple('PickmanMr', [ - 'iid', 'title', 'web_url', 'source_branch', 'description' -]) + 'iid', 'title', 'web_url', 'source_branch', 'description', + 'has_conflicts', 'needs_rebase' +], defaults=[False, False]) # Comment info returned by get_mr_comments() MrComment = namedtuple('MrComment', [ @@ -232,12 +234,22 @@ def get_pickman_mrs(remote, state='opened'): pickman_mrs = [] for merge_req in mrs: if '[pickman]' in merge_req.title: + # Check merge status - detailed_merge_status is newer API + detailed_status = getattr(merge_req, 'detailed_merge_status', '') + needs_rebase = detailed_status == 'need_rebase' + # Also check diverged_commits_count as fallback + if not needs_rebase: + diverged = getattr(merge_req, 'diverged_commits_count', 0) + needs_rebase = diverged and diverged > 0 + pickman_mrs.append(PickmanMr( iid=merge_req.iid, title=merge_req.title, web_url=merge_req.web_url, source_branch=merge_req.source_branch, description=merge_req.description or '', + has_conflicts=getattr(merge_req, 'has_conflicts', False), + needs_rebase=needs_rebase, )) return pickman_mrs except gitlab.exceptions.GitlabError as exc: -- 2.43.0