[Concept,v2,1/2] pickman: Add git fetch before MR operations

Message ID 20251217192831.1308453-2-sjg@u-boot.org
State New
Headers
Series pickman: Improve handling of rebasing |

Commit Message

Simon Glass Dec. 17, 2025, 7:28 p.m. UTC
  From: Simon Glass <simon.glass@canonical.com>

Add git fetch to ensure pickman uses the latest remote state:
- Before creating a new MR, fetch the remote first
- Before processing review comments, fetch the remote

This ensures that when creating MRs, the base branch (ci/master) is
up to date, and when the agent handles review comments that request
a rebase, it has the latest commits available.

Also update the agent prompt to use the configured remote/target for
rebase operations instead of a hardcoded value.

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

(no changes since v1)

 tools/pickman/agent.py   | 12 ++++++++----
 tools/pickman/control.py | 14 ++++++++++++--
 tools/pickman/ftest.py   | 15 ++++++++-------
 3 files changed, 28 insertions(+), 13 deletions(-)
  

Patch

diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py
index 26f4ac3afd9..bb922b03b13 100644
--- a/tools/pickman/agent.py
+++ b/tools/pickman/agent.py
@@ -136,7 +136,8 @@  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):
+async def run_review_agent(mr_iid, branch_name, comments, remote, target='master',
+                           repo_path=None):
     """Run the Claude agent to handle MR comments
 
     Args:
@@ -144,6 +145,7 @@  async def run_review_agent(mr_iid, branch_name, comments, remote, repo_path=None
         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
         repo_path (str): Path to repository (defaults to current directory)
 
     Returns:
@@ -182,7 +184,7 @@  Important:
 - 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>
+- If rebasing is requested, use: git rebase --keep-empty {remote}/{target}
   This preserves empty merge commits which are important for tracking
 """
 
@@ -208,7 +210,8 @@  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',
+                       repo_path=None):
     """Synchronous wrapper for running the review agent
 
     Args:
@@ -216,6 +219,7 @@  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
         repo_path (str): Path to repository (defaults to current directory)
 
     Returns:
@@ -223,4 +227,4 @@  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, repo_path))
diff --git a/tools/pickman/control.py b/tools/pickman/control.py
index b07ef703ba2..dec2c68883f 100644
--- a/tools/pickman/control.py
+++ b/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,10 +658,15 @@  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
     """
+    # 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:
@@ -692,6 +697,7 @@  def process_mr_reviews(remote, mrs, dbs):
             merge_req.source_branch,
             unresolved,
             remote,
+            target,
         )
 
         if success:
@@ -911,13 +917,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
diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py
index 769813122fb..9d7786541c3 100644
--- a/tools/pickman/ftest.py
+++ b/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