pickman: Auto-detect when MRs need rebasing

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

Series-to: concept
Series-links: 2:81 1:80
Cover-letter:
pickman: Improve handling of rebasing
This little series includes a few patches to ensure that rebases are
made against -master and detection for needing a rebase without the user
having explicitly request it.

It also passes the previous context to the agent when handling reviews,
which should improve its response.
END

Co-developed-by: Claude <noreply@anthropic.com>
Signed-off-by: Simon Glass <simon.glass@canonical.com>
Series-version: 2

tools/pickman/agent.py

@@ -136,14 +136,20 @@ def cherry_pick_commits(commits, source, branch_name, repo_path=None):
                                              repo_path))
 
 
-async def run_review_agent(mr_iid, branch_name, comments, remote, repo_path=None):
-    """Run the Claude agent to handle MR comments
+async def run_review_agent(mr_iid, branch_name, comments, remote, target='master',
+                           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
         branch_name (str): Source branch name
         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:
@@ -155,35 +161,75 @@ async def run_review_agent(mr_iid, branch_name, comments, remote, repo_path=None
     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)")
 
-    prompt = f"""Review comments on merge request !{mr_iid} (branch: {branch_name}):
+    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):
+
+{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 <base>
-  This preserves empty merge commits which are important for tracking
+- Always use -o ci.skip when pushing to avoid duplicate pipelines
 """
 
     options = ClaudeAgentOptions(
@@ -191,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 = []
@@ -208,7 +254,9 @@ Important:
         return False, '\n\n'.join(conversation_log)
 
 
-def handle_mr_comments(mr_iid, branch_name, comments, remote, repo_path=None):
+def handle_mr_comments(mr_iid, branch_name, comments, remote, target='master',
+                       needs_rebase=False, has_conflicts=False,
+                       mr_description='', repo_path=None):
     """Synchronous wrapper for running the review agent
 
     Args:
@@ -216,6 +264,10 @@ def handle_mr_comments(mr_iid, branch_name, comments, remote, repo_path=None):
         branch_name (str): Source branch name
         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:
@@ -223,4 +275,5 @@ def handle_mr_comments(mr_iid, branch_name, comments, remote, repo_path=None):
             conversation_log is the agent's output text
     """
     return asyncio.run(run_review_agent(mr_iid, branch_name, comments, remote,
-                                        repo_path))
+                                        target, needs_rebase, has_conflicts,
+                                        mr_description, repo_path))

tools/pickman/control.py

@@ -648,7 +648,7 @@ def do_commit_source(args, dbs):
     return 0
 
 
-def process_mr_reviews(remote, mrs, dbs):
+def process_mr_reviews(remote, mrs, dbs, target='master'):
     """Process review comments on open MRs
 
     Checks each MR for unresolved comments and uses Claude agent to address
@@ -658,17 +658,25 @@ def process_mr_reviews(remote, mrs, dbs):
         remote (str): Remote name
         mrs (list): List of MR dicts from get_open_pickman_mrs()
         dbs (Database): Database instance for tracking processed comments
+        target (str): Target branch for rebase operations
 
     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])
+
     processed = 0
 
     for merge_req in mrs:
         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 = []
@@ -678,20 +686,35 @@ def process_mr_reviews(remote, mrs, dbs):
             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:
@@ -720,6 +743,11 @@ def process_mr_reviews(remote, mrs, dbs):
             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
 
 
@@ -911,13 +939,17 @@ def do_step(args, dbs):
             tout.info(f"  !{merge_req.iid}: {merge_req.title}")
 
         # Process any review comments on open MRs
-        process_mr_reviews(remote, mrs, dbs)
+        process_mr_reviews(remote, mrs, dbs, args.target)
 
         tout.info('')
         tout.info('Not creating new MR while others are pending')
         return 0
 
     # No pending MRs, run apply with push
+    # First fetch to get latest remote state
+    tout.info(f'Fetching {remote}...')
+    run_git(['fetch', remote])
+
     tout.info('No pending pickman MRs, creating new one...')
     args.push = True
     args.branch = None  # Let do_apply generate branch name

tools/pickman/ftest.py

@@ -1900,13 +1900,14 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase):
                                      resolved=False),
             ]
 
-            with mock.patch.object(gitlab_api, 'get_mr_comments',
-                                   return_value=comments):
-                with mock.patch.object(agent, 'handle_mr_comments',
-                                       return_value=(True, 'Done')) 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)
+            with mock.patch.object(control, 'run_git'):
+                with mock.patch.object(gitlab_api, 'get_mr_comments',
+                                       return_value=comments):
+                    with mock.patch.object(agent, 'handle_mr_comments',
+                                           return_value=(True, 'Done')) 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 only receive the new comment
             call_args = mock_agent.call_args
@@ -1916,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."""

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: