From patchwork Wed Dec 17 19:18:18 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 968 Return-Path: X-Original-To: u-boot-concept@u-boot.org Delivered-To: u-boot-concept@u-boot.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765999141; bh=BZxASEUqpzkXRFwqvAajLe9nS+IE50hORee8exoL6d4=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=Jfc+N9NwHlUxJgbRR9uHoIGpWs2AxJKs3+eHX9QP5Tg0fFf8ux7fxg9x6rmRV5sae WnPnGkI/LO+/m+F4BqgA1zw/YabpqUYMiThrfD0THKSCDAcQ8lDBeIEsR+8jD4LZGC 1yt3A8+p+FwepOSDAOAts4GqI0W8Og++iD0tnKHjyZ7kWCe2ClmK/EKiv1tg7kQMDJ Cd+KFQtiDwZQLkm/YGm8APZdFIWQMwUhqJlS+L2q8aKW5F9DICOZ3+TXtbHyxUbBUZ /Ryq32cT7UBfvyNJ7kAPIu7DPly1Q/Q9XEsDtFecpm8CT27YWyCG5Tfx8gcXflg5/G W69iTubk15OIg== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id F058668C14 for ; Wed, 17 Dec 2025 12:19:01 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10024) with ESMTP id Ihf69Ow5bjEW for ; Wed, 17 Dec 2025 12:19:01 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765999140; bh=BZxASEUqpzkXRFwqvAajLe9nS+IE50hORee8exoL6d4=; h=From:To:Date:In-Reply-To:References:CC:Subject:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=avjenQzY1RFPsJZtvixateqsNZITT9/eh/UQNLSUSUXqUo3xT3chQO0PSGrMrLhrw Rf1Ja1SsS1tZJRqMnsRx6ZeDx5OE614LzdQtm2fVdTlNSx23MefRPHe6EZi1N0jJi5 ki1ZKFaxQ8oUZsS3CAHmpHLz0u/4OvN/CqQN8DUJENWFXW4XszTRt1Mvluy62k9MD/ wWOvHJztV5vMKlnF49FPFdavSwS9bbbBOH9dWfbI/UPPYCNOaN3Ls4Bn3fBsSHdQbM d8eJO9H06YsnRC+wlvoJ9ijeuLWnnPCZConP7htch5utzacM8tHCks7Qs1i9CeIeHk /xBQ/b+JN8UmA== Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id EC79368B26 for ; Wed, 17 Dec 2025 12:19:00 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765999139; bh=fS4kTEGV5vxA5gYYn6YmAMjxmh1yw59YXH2UvfWKZ0A=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=sJLR1clNwPqs0VPubfJgkIjobco9UycEER+M4z9R6GFN1quaiO/67sYFRpnd3lzBu DQoNl52xizxvP6cR88kuGSnnuOOrzphh4u16jIJYqqWT+frMFTrrsPnhcYnhZuqBlb qvLILXATDm/i9E2K+MgH5ncT75bAJzwiZtdgXl0Olc6k61TVhfsWuy4Dcnb9j7MP1h VyESw1yMgpRCz04xAtoVB3MfM/2oRAdBgfeNVyzRSoC2mUHDwMn6Ssc6kLEsIN/TjL A6uXlW5b4Ojk8yj/uA+87xlJ0QaHwgBTrHOxTkRQCsuBYKpfQU+8OggEzJ8jACvVdq XRnZiFTGtMWyg== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id A9D7568C14; Wed, 17 Dec 2025 12:18:59 -0700 (MST) X-Virus-Scanned: Debian amavis at Received: from mail.u-boot.org ([127.0.0.1]) by localhost (mail.u-boot.org [127.0.0.1]) (amavis, port 10026) with ESMTP id vZIV3Dz46Dcv; Wed, 17 Dec 2025 12:18:59 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765999135; bh=xAJz2aWmDWIZKDbJanM4+avpZtxe+/l01TdvAmbKjNE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=tDG1LqWNRWeCbHOhjEGDlCwXJSB+C1xJxcLlhdEWL8ND/SkmVqnk2jkE90EwquJ03 Y6AvCZTpzYLtlxRCXfCRTF0PIivsDFQfzCFPzdNorjS+1EpIB3NxWqSibERzg2gBrQ iuT+IlnaOO0eO9v4uhQtG+6f+JIfX8EEjxOAUlv9wBkqnK8jzhFhfBeYp6HyDbP9vI plWi9Mm1AZcO8C4HqDta28vtQO2oUi6h6A0iGkDxehdKvTQQ090VJsHyh7WJ2/nDWr WklgFoMnlOmdtpHVSz+v3el2TnFb4l8gpRaaUq1iYU1soZGypmR2lL8Kn+Gn4ttBvy 9P+gDew3giwgw== Received: from u-boot.org (unknown [73.34.74.121]) by mail.u-boot.org (Postfix) with ESMTPSA id 1423068C12; Wed, 17 Dec 2025 12:18:55 -0700 (MST) From: Simon Glass To: U-Boot Concept Date: Wed, 17 Dec 2025 12:18:18 -0700 Message-ID: <20251217191834.626062-3-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20251217191834.626062-1-sjg@u-boot.org> References: <20251217191834.626062-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: BT4EW4UCQIOR73IUSK4B3AVOSBAM5M67 X-Message-ID-Hash: BT4EW4UCQIOR73IUSK4B3AVOSBAM5M67 X-MailFrom: sjg@u-boot.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Simon Glass , Claude X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 2/2] pickman: Auto-detect when MRs need rebasing List-Id: Discussion and patches related to U-Boot Concept Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: From: Simon Glass 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 Signed-off-by: Simon Glass --- 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: