From: Simon Glass <simon.glass@canonical.com> Update the review agent to: - Create local branch with version suffix (-v2, -v3, etc.) - Force push to original remote branch to update existing MR - Use --keep-empty when rebasing to preserve empty merge commits After processing comments: - Mark them as processed in database to avoid reprocessing - Update MR description with agent conversation log - Append review handling to .pickman-history Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/README.rst | 14 +++- tools/pickman/agent.py | 13 ++-- tools/pickman/control.py | 98 ++++++++++++++++++++++---- tools/pickman/ftest.py | 146 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 251 insertions(+), 20 deletions(-) diff --git a/tools/pickman/README.rst b/tools/pickman/README.rst index a9d6809d074..d94d399ab4d 100644 --- a/tools/pickman/README.rst +++ b/tools/pickman/README.rst @@ -119,8 +119,18 @@ To check open MRs for comments and address them:: ./tools/pickman/pickman review This lists open pickman MRs (those with ``[pickman]`` in the title), checks each -for unresolved comments, and uses a Claude agent to address them. The agent will -make code changes based on the feedback and push an updated branch. +for unresolved comments, and uses a Claude agent to address them. The agent will: + +- Make code changes based on the feedback +- Create a local branch with version suffix (e.g., ``cherry-abc123-v2``) +- Force push to the original remote branch to update the existing MR +- Use ``--keep-empty`` when rebasing to preserve empty merge commits + +After processing, pickman: + +- Marks comments as processed in the database (to avoid reprocessing) +- Updates the MR description with the agent's conversation log +- Appends the review handling to ``.pickman-history`` Options for the review command: diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py index 932d61be4d7..e63248a1150 100644 --- a/tools/pickman/agent.py +++ b/tools/pickman/agent.py @@ -168,16 +168,19 @@ Steps to follow: 3. For each actionable comment: - Make the requested changes to the code - Amend the relevant commit or create a fixup commit -4. Run 'buildman -L --board sandbox -w -o /tmp/pickman' to verify the build -5. Create a new branch with suffix '-v2' (or increment existing version) -6. Push the new branch: git push {remote} <new-branch-name> +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 Important: - Keep changes minimal and focused on addressing the comments - If a comment is unclear or cannot be addressed, note this in your report -- Do not force push to the original branch -- The new branch name should be: {branch_name}-v2 (or -v3, -v4 etc if needed) +- 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 """ options = ClaudeAgentOptions( diff --git a/tools/pickman/control.py b/tools/pickman/control.py index acb3b80a4a7..a6c371f0500 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -574,15 +574,16 @@ def do_commit_source(args, dbs): return 0 -def process_mr_reviews(remote, mrs): +def process_mr_reviews(remote, mrs, dbs): """Process review comments on open MRs Checks each MR for unresolved comments and uses Claude agent to address - them. + them. Updates MR description and .pickman-history with conversation log. Args: remote (str): Remote name mrs (list): List of MR dicts from get_open_pickman_mrs() + dbs (Database): Database instance for tracking processed comments Returns: int: Number of MRs with comments processed @@ -590,35 +591,106 @@ def process_mr_reviews(remote, mrs): processed = 0 for merge_req in mrs: - comments = gitlab_api.get_mr_comments(remote, merge_req.iid) + mr_iid = merge_req.iid + comments = gitlab_api.get_mr_comments(remote, mr_iid) if comments is None: continue - # Filter to unresolved comments - unresolved = [c for c in comments if not c.resolved] + # Filter to unresolved comments that haven't been processed + unresolved = [] + for com in comments: + if com.resolved: + continue + if dbs.comment_is_processed(mr_iid, com.id): + continue + unresolved.append(com) if not unresolved: continue tout.info('') - tout.info(f"MR !{merge_req.iid} has {len(unresolved)} comment(s):") + 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 - success, _ = agent.handle_mr_comments( - merge_req.iid, + success, conversation_log = agent.handle_mr_comments( + mr_iid, merge_req.source_branch, unresolved, remote, ) - if not success: - tout.error(f"Failed to handle comments for MR !{merge_req.iid}") + + if success: + # Mark comments as processed + for comment in unresolved: + dbs.comment_mark_processed(mr_iid, comment.id) + dbs.commit() + + # Update MR description with comments and conversation log + old_desc = merge_req.description + comment_summary = '\n'.join( + f"- [{c.author}]: {c.body}" + for c in unresolved + ) + new_desc = (f"{old_desc}\n\n### Review response\n\n" + f"**Comments addressed:**\n{comment_summary}\n\n" + f"**Response:**\n{conversation_log}") + gitlab_api.update_mr_description(remote, mr_iid, new_desc) + + # Update .pickman-history + update_history_with_review(merge_req.source_branch, + unresolved, conversation_log) + + tout.info(f'Updated MR !{mr_iid} description and history') + else: + tout.error(f"Failed to handle comments for MR !{mr_iid}") processed += 1 return processed -def do_review(args, dbs): # pylint: disable=unused-argument +def update_history_with_review(branch_name, comments, conversation_log): + """Append review handling to .pickman-history + + Args: + branch_name (str): Branch name for the MR + comments (list): List of comments that were addressed + conversation_log (str): Agent conversation log + """ + comment_summary = '\n'.join( + f"- [{c.author}]: {c.body[:100]}..." + for c in comments + ) + + entry = f"""### Review: {date.today()} + +Branch: {branch_name} + +Comments addressed: +{comment_summary} + +### Conversation log +{conversation_log} + +--- + +""" + + # Append to history file + existing = '' + if os.path.exists(HISTORY_FILE): + with open(HISTORY_FILE, 'r', encoding='utf-8') as fhandle: + existing = fhandle.read() + + with open(HISTORY_FILE, 'w', encoding='utf-8') as fhandle: + fhandle.write(existing + entry) + + # Commit the history file + run_git(['add', '-f', HISTORY_FILE]) + run_git(['commit', '-m', f'pickman: Record review handling for {branch_name}']) + + +def do_review(args, dbs): """Check open pickman MRs and handle comments Lists open MRs created by pickman, checks for human comments, and uses @@ -646,7 +718,7 @@ def do_review(args, dbs): # pylint: disable=unused-argument for merge_req in mrs: tout.info(f" !{merge_req.iid}: {merge_req.title}") - process_mr_reviews(remote, mrs) + process_mr_reviews(remote, mrs, dbs) return 0 @@ -765,7 +837,7 @@ 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) + process_mr_reviews(remote, mrs, dbs) tout.info('') tout.info('Not creating new MR while others are pending') diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index ec9e7e91d65..3fbb4e20dd7 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -8,6 +8,8 @@ import argparse import os +import shutil +import subprocess import sys import tempfile import unittest @@ -23,6 +25,7 @@ from u_boot_pylib import terminal from u_boot_pylib import tout from pickman import __main__ as pickman +from pickman import agent from pickman import control from pickman import database from pickman import gitlab_api @@ -1586,6 +1589,149 @@ class TestReview(unittest.TestCase): self.assertEqual(ret, 1) +class TestUpdateHistoryWithReview(unittest.TestCase): + """Tests for update_history_with_review function.""" + + def setUp(self): + """Set up test fixtures.""" + self.test_dir = tempfile.mkdtemp() + self.orig_dir = os.getcwd() + os.chdir(self.test_dir) + + # Initialize git repo + subprocess.run(['git', 'init'], check=True, capture_output=True) + subprocess.run(['git', 'config', 'user.email', 'test@test.com'], + check=True, capture_output=True) + subprocess.run(['git', 'config', 'user.name', 'Test'], + check=True, capture_output=True) + + def tearDown(self): + """Clean up test fixtures.""" + os.chdir(self.orig_dir) + shutil.rmtree(self.test_dir) + + def test_update_history_with_review(self): + """Test that review handling is appended to history.""" + comments = [ + gitlab_api.MrComment(id=1, author='reviewer1', + body='Please fix the indentation here', + created_at='2025-01-01', resolvable=True, + resolved=False), + gitlab_api.MrComment(id=2, author='reviewer2', body='Add a docstring', + created_at='2025-01-01', resolvable=True, + resolved=False), + ] + conversation_log = 'Fixed indentation and added docstring.' + + control.update_history_with_review('cherry-abc123', comments, + conversation_log) + + # Check history file was created + self.assertTrue(os.path.exists(control.HISTORY_FILE)) + + with open(control.HISTORY_FILE, 'r', encoding='utf-8') as fhandle: + content = fhandle.read() + + self.assertIn('### Review:', content) + self.assertIn('Branch: cherry-abc123', content) + self.assertIn('reviewer1', content) + self.assertIn('reviewer2', content) + self.assertIn('Fixed indentation', content) + + def test_update_history_appends(self): + """Test that review handling appends to existing history.""" + # Create existing history + with open(control.HISTORY_FILE, 'w', encoding='utf-8') as fhandle: + fhandle.write('Existing history content\n') + subprocess.run(['git', 'add', control.HISTORY_FILE], + check=True, capture_output=True) + subprocess.run(['git', 'commit', '-m', 'Initial'], + check=True, capture_output=True) + + comments = [gitlab_api.MrComment(id=1, author='reviewer', body='Fix this', + created_at='2025-01-01', resolvable=True, + resolved=False)] + control.update_history_with_review('cherry-xyz', comments, 'Fixed it') + + with open(control.HISTORY_FILE, 'r', encoding='utf-8') as fhandle: + content = fhandle.read() + + self.assertIn('Existing history content', content) + self.assertIn('### Review:', content) + + +class TestProcessMrReviewsCommentTracking(unittest.TestCase): + """Tests for comment tracking in process_mr_reviews.""" + + def setUp(self): + """Set up test fixtures.""" + fd, self.db_path = tempfile.mkstemp(suffix='.db') + os.close(fd) + os.unlink(self.db_path) + self.test_dir = tempfile.mkdtemp() + self.orig_dir = os.getcwd() + os.chdir(self.test_dir) + + # Initialize git repo + subprocess.run(['git', 'init'], check=True, capture_output=True) + subprocess.run(['git', 'config', 'user.email', 'test@test.com'], + check=True, capture_output=True) + subprocess.run(['git', 'config', 'user.name', 'Test'], + check=True, capture_output=True) + + def tearDown(self): + """Clean up test fixtures.""" + os.chdir(self.orig_dir) + shutil.rmtree(self.test_dir) + if os.path.exists(self.db_path): + os.unlink(self.db_path) + database.Database.instances.clear() + + def test_skips_processed_comments(self): + """Test that already-processed comments are skipped.""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + + # Mark comment as processed + dbs.comment_mark_processed(100, 1) + dbs.commit() + + mrs = [gitlab_api.PickmanMr( + iid=100, + title='[pickman] Test MR', + source_branch='cherry-test', + description='Test', + web_url='https://gitlab.com/mr/100', + )] + + # Comment 1 is processed, comment 2 is new + comments = [ + gitlab_api.MrComment(id=1, author='reviewer', body='Old comment', + created_at='2025-01-01', resolvable=True, + resolved=False), + gitlab_api.MrComment(id=2, author='reviewer', body='New comment', + created_at='2025-01-01', resolvable=True, + 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) + + # Agent should only receive the new comment + call_args = mock_agent.call_args + passed_comments = call_args[0][2] + self.assertEqual(len(passed_comments), 1) + self.assertEqual(passed_comments[0].id, 2) + + dbs.close() + + class TestParsePoll(unittest.TestCase): """Tests for poll command argument parsing.""" -- 2.43.0