@@ -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:
@@ -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(
@@ -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')
@@ -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."""