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(-) 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 -- 2.43.0