From: Simon Glass <simon.glass@canonical.com> Add do_review() command that lists open pickman MRs and checks each for human comments. Uses a Claude agent to address actionable comments. Also add supporting infrastructure: - gitlab_api: get_open_pickman_mrs(), get_mr_comments(), reply_to_mr() - agent: run_review_agent(), handle_mr_comments() - History file support for recording cherry-pick conversations - Return conversation_log from cherry_pick agent for MR descriptions Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/README.rst | 12 ++++ tools/pickman/__main__.py | 5 ++ tools/pickman/agent.py | 87 ++++++++++++++++++++++++ tools/pickman/control.py | 77 +++++++++++++++++++++ tools/pickman/ftest.py | 38 +++++++++++ tools/pickman/gitlab_api.py | 130 ++++++++++++++++++++++++++++++++++++ 6 files changed, 349 insertions(+) diff --git a/tools/pickman/README.rst b/tools/pickman/README.rst index d15b15f3331..f17ab734580 100644 --- a/tools/pickman/README.rst +++ b/tools/pickman/README.rst @@ -80,6 +80,18 @@ After successfully applying commits, update the database to record progress:: This updates the last cherry-picked commit for the source branch, so subsequent ``next-set`` and ``apply`` commands will start from the new position. +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. + +Options for the review command: + +- ``-r, --remote``: Git remote (default: ci) + Requirements ------------ diff --git a/tools/pickman/__main__.py b/tools/pickman/__main__.py index 1693af991ac..17f7d72a619 100755 --- a/tools/pickman/__main__.py +++ b/tools/pickman/__main__.py @@ -57,6 +57,11 @@ def parse_args(argv): help='Show next set of commits to cherry-pick') next_set.add_argument('source', help='Source branch name') + review_cmd = subparsers.add_parser('review', + help='Check open MRs and handle comments') + review_cmd.add_argument('-r', '--remote', default='ci', + help='Git remote (default: ci)') + subparsers.add_parser('test', help='Run tests') return parser.parse_args(argv) diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py index 3b4b96838b7..76cf8f98ec3 100644 --- a/tools/pickman/agent.py +++ b/tools/pickman/agent.py @@ -132,3 +132,90 @@ def cherry_pick_commits(commits, source, branch_name, repo_path=None): """ return asyncio.run(run(commits, source, branch_name, repo_path)) + + +async def run_review_agent(mr_iid, branch_name, comments, remote, repo_path=None): + """Run the Claude agent to handle MR comments + + Args: + mr_iid (int): Merge request IID + branch_name (str): Source branch name + comments (list): List of comment dicts with 'author', 'body' keys + remote (str): Git remote name + repo_path (str): Path to repository (defaults to current directory) + + Returns: + bool: True on success, False on failure + """ + if not check_available(): + return False + + 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 + ) + + prompt = f"""Review comments on merge request !{mr_iid} (branch: {branch_name}): + +{comment_text} + +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 +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> +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) +""" + + options = ClaudeAgentOptions( + allowed_tools=['Bash', 'Read', 'Grep', 'Edit', 'Write'], + cwd=repo_path, + ) + + tout.info(f'Starting Claude agent to handle {len(comments)} comment(s)...') + tout.info('') + + conversation_log = [] + try: + async for message in query(prompt=prompt, options=options): + if hasattr(message, 'content'): + for block in message.content: + if hasattr(block, 'text'): + print(block.text) + conversation_log.append(block.text) + return True, '\n\n'.join(conversation_log) + except (RuntimeError, ValueError, OSError) as exc: + tout.error(f'Agent failed: {exc}') + return False, '\n\n'.join(conversation_log) + + +def handle_mr_comments(mr_iid, branch_name, comments, remote, repo_path=None): + """Synchronous wrapper for running the review agent + + Args: + mr_iid (int): Merge request IID + branch_name (str): Source branch name + comments (list): List of comment dicts + remote (str): Git remote name + repo_path (str): Path to repository (defaults to current directory) + + Returns: + tuple: (success, conversation_log) where success is bool and + conversation_log is the agent's output text + """ + return asyncio.run(run_review_agent(mr_iid, branch_name, comments, remote, + repo_path)) diff --git a/tools/pickman/control.py b/tools/pickman/control.py index fe840ee2740..1216b27bc9b 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -436,6 +436,82 @@ def do_commit_source(args, dbs): return 0 +def process_mr_reviews(remote, mrs): + """Process review comments on open MRs + + Checks each MR for unresolved comments and uses Claude agent to address + them. + + Args: + remote (str): Remote name + mrs (list): List of MR dicts from get_open_pickman_mrs() + + Returns: + int: Number of MRs with comments processed + """ + processed = 0 + + for merge_req in mrs: + comments = gitlab_api.get_mr_comments(remote, merge_req['iid']) + if comments is None: + continue + + # Filter to unresolved comments + unresolved = [c for c in comments if not c.get('resolved', True)] + if not unresolved: + continue + + tout.info('') + tout.info(f"MR !{merge_req['iid']} has {len(unresolved)} 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'], + merge_req['source_branch'], + unresolved, + remote, + ) + if not success: + tout.error(f"Failed to handle comments for MR !{merge_req['iid']}") + processed += 1 + + return processed + + +def do_review(args, dbs): # pylint: disable=unused-argument + """Check open pickman MRs and handle comments + + Lists open MRs created by pickman, checks for human comments, and uses + Claude agent to address them. + + Args: + args (Namespace): Parsed arguments with 'remote' attribute + dbs (Database): Database instance + + Returns: + int: 0 on success, 1 on failure + """ + remote = args.remote + + # Get open pickman MRs + mrs = gitlab_api.get_open_pickman_mrs(remote) + if mrs is None: + return 1 + + if not mrs: + tout.info('No open pickman MRs found') + return 0 + + tout.info(f'Found {len(mrs)} open pickman MR(s):') + for merge_req in mrs: + tout.info(f" !{merge_req['iid']}: {merge_req['title']}") + + process_mr_reviews(remote, mrs) + + return 0 + def do_test(args, dbs): # pylint: disable=unused-argument """Run tests for this module. @@ -463,6 +539,7 @@ COMMANDS = { 'compare': do_compare, 'list-sources': do_list_sources, 'next-set': do_next_set, + 'review': do_review, 'test': do_test, } diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 9b445173b3b..0e722bff48c 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -1105,5 +1105,43 @@ class TestParseApplyWithPush(unittest.TestCase): self.assertEqual(args.target, 'main') +class TestParseReview(unittest.TestCase): + """Tests for review command argument parsing.""" + + def test_parse_review_defaults(self): + """Test parsing review command with defaults.""" + args = pickman.parse_args(['review']) + self.assertEqual(args.cmd, 'review') + self.assertEqual(args.remote, 'ci') + + def test_parse_review_with_remote(self): + """Test parsing review command with custom remote.""" + args = pickman.parse_args(['review', '-r', 'origin']) + self.assertEqual(args.cmd, 'review') + self.assertEqual(args.remote, 'origin') + + +class TestReview(unittest.TestCase): + """Tests for review command.""" + + def test_review_no_mrs(self): + """Test review when no open MRs found.""" + with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + return_value=[]): + args = argparse.Namespace(cmd='review', remote='ci') + ret = control.do_review(args, None) + + self.assertEqual(ret, 0) + + def test_review_gitlab_error(self): + """Test review when GitLab API returns error.""" + with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + return_value=None): + args = argparse.Namespace(cmd='review', remote='ci') + ret = control.do_review(args, None) + + self.assertEqual(ret, 1) + + if __name__ == '__main__': unittest.main() diff --git a/tools/pickman/gitlab_api.py b/tools/pickman/gitlab_api.py index 6cd9c26f3de..ca239c41271 100644 --- a/tools/pickman/gitlab_api.py +++ b/tools/pickman/gitlab_api.py @@ -152,6 +152,136 @@ def create_mr(host, proj_path, source, target, title, desc=''): return None +def get_open_pickman_mrs(remote): + """Get open merge requests created by pickman + + Args: + remote (str): Remote name + + Returns: + list: List of dicts with 'iid', 'title', 'web_url', 'source_branch' keys, + or None on failure + """ + if not check_available(): + return None + + token = get_token() + if not token: + tout.error('GITLAB_TOKEN environment variable not set') + return None + + remote_url = get_remote_url(remote) + host, proj_path = parse_url(remote_url) + + if not host or not proj_path: + tout.error(f"Could not parse GitLab URL from remote '{remote}'") + return None + + try: + glab = gitlab.Gitlab(f'https://{host}', private_token=token) + project = glab.projects.get(proj_path) + + mrs = project.mergerequests.list(state='opened', get_all=True) + pickman_mrs = [] + for merge_req in mrs: + if '[pickman]' in merge_req.title: + pickman_mrs.append({ + 'iid': merge_req.iid, + 'title': merge_req.title, + 'web_url': merge_req.web_url, + 'source_branch': merge_req.source_branch, + }) + return pickman_mrs + except gitlab.exceptions.GitlabError as exc: + tout.error(f'GitLab API error: {exc}') + return None + + +def get_mr_comments(remote, mr_iid): + """Get human comments on a merge request (excluding bot/system notes) + + Args: + remote (str): Remote name + mr_iid (int): Merge request IID + + Returns: + list: List of dicts with 'id', 'author', 'body', 'created_at', + 'resolvable', 'resolved' keys, or None on failure + """ + if not check_available(): + return None + + token = get_token() + if not token: + tout.error('GITLAB_TOKEN environment variable not set') + return None + + remote_url = get_remote_url(remote) + host, proj_path = parse_url(remote_url) + + if not host or not proj_path: + return None + + try: + glab = gitlab.Gitlab(f'https://{host}', private_token=token) + project = glab.projects.get(proj_path) + merge_req = project.mergerequests.get(mr_iid) + + comments = [] + for note in merge_req.notes.list(get_all=True): + # Skip system notes (merge status, etc.) + if note.system: + continue + comments.append({ + 'id': note.id, + 'author': note.author['username'], + 'body': note.body, + 'created_at': note.created_at, + 'resolvable': getattr(note, 'resolvable', False), + 'resolved': getattr(note, 'resolved', False), + }) + return comments + except gitlab.exceptions.GitlabError as exc: + tout.error(f'GitLab API error: {exc}') + return None + + +def reply_to_mr(remote, mr_iid, message): + """Post a reply to a merge request + + Args: + remote (str): Remote name + mr_iid (int): Merge request IID + message (str): Reply message + + Returns: + bool: True on success + """ + if not check_available(): + return False + + token = get_token() + if not token: + tout.error('GITLAB_TOKEN environment variable not set') + return False + + remote_url = get_remote_url(remote) + host, proj_path = parse_url(remote_url) + + if not host or not proj_path: + return False + + try: + glab = gitlab.Gitlab(f'https://{host}', private_token=token) + project = glab.projects.get(proj_path) + merge_req = project.mergerequests.get(mr_iid) + merge_req.notes.create({'body': message}) + return True + except gitlab.exceptions.GitlabError as exc: + tout.error(f'GitLab API error: {exc}') + return False + + def push_and_create_mr(remote, branch, target, title, desc=''): """Push a branch and create a merge request -- 2.43.0