From patchwork Wed Dec 17 02:28:04 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Glass X-Patchwork-Id: 958 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=1765938589; bh=Nb1Am1zGE43Wz+hpZdEleyAyKAFJjGHEXx0++pbfs6k=; 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=UT1tpAyvX8sJRUaSXUzABrlFH8LDeUKZSGji2kpHgNvJbrT3iWttDFzvdTN3YgjKo xoKZeUkjH8mImYBIPhX0HV3XxwiDpcRarTpFjH18cnVJhWBH+dqaSkQYUfj5YSoO1e GKHM5CpqVhS07LIYwXY7w//8v8D+TMMDEdLY7QdIV6NUTUxtOmOwTp1r82oDtCrEey QrLu5wSh9NivOQ1lVBR6hmIziCQBo+sWt3bbs/AUimXFJkifZDpiCdR0hxTzyrjfvw iW1ZRBaD1WSGHvRw4wQRanYlYxp9kNA9mGitFePCX3t3nohsOKpLFQJ37cv/UP9BtN 9F2Q7Xh4U+bDA== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id D1F5E68BC7 for ; Tue, 16 Dec 2025 19:29:49 -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 w6zlCZB1p6Of for ; Tue, 16 Dec 2025 19:29:49 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765938589; bh=Nb1Am1zGE43Wz+hpZdEleyAyKAFJjGHEXx0++pbfs6k=; 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=UT1tpAyvX8sJRUaSXUzABrlFH8LDeUKZSGji2kpHgNvJbrT3iWttDFzvdTN3YgjKo xoKZeUkjH8mImYBIPhX0HV3XxwiDpcRarTpFjH18cnVJhWBH+dqaSkQYUfj5YSoO1e GKHM5CpqVhS07LIYwXY7w//8v8D+TMMDEdLY7QdIV6NUTUxtOmOwTp1r82oDtCrEey QrLu5wSh9NivOQ1lVBR6hmIziCQBo+sWt3bbs/AUimXFJkifZDpiCdR0hxTzyrjfvw iW1ZRBaD1WSGHvRw4wQRanYlYxp9kNA9mGitFePCX3t3nohsOKpLFQJ37cv/UP9BtN 9F2Q7Xh4U+bDA== Received: from mail.u-boot.org (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id BE25368AEE for ; Tue, 16 Dec 2025 19:29:49 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765938588; bh=OvAOCAJiY1sV03JAaRsePysbUqcBBOcDR/PHV0iyTlg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=qGTwnZfbsfONe5uJijcUPyCO/u4ma+hp0o8FuqdNkuI5TyODyY/N3uKTL8g4ujUmC VZWjvp0CO+pDDl19DnPzcJItFroO+Z8UDHN5ABuIk8E01c+HAN/FMIzcANsf+AMQJ+ K/slpEChap22pAS/khWney/6pM/Ujw/2VUdbmpMA9VmrfDwSf8qw0LT2SFhznTmeO+ npWJ/BlOjuIwWtqduaQyp0LaXcvwnHwtqWiw8lpeV1RgmfmVvvtZaKkDCph6WdCsFx OqQzp2rmDe8U6zzQuv3kvuax3DM2fdsiYR/+qjSYIVkdMhuY0mZNVNNnndDSbkSAN8 pTMM1jXeZzvNw== Received: from localhost (localhost [127.0.0.1]) by mail.u-boot.org (Postfix) with ESMTP id DEF5F6897B; Tue, 16 Dec 2025 19:29:48 -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 YbYNgnOhVihl; Tue, 16 Dec 2025 19:29:48 -0700 (MST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=u-boot.org; s=default; t=1765938584; bh=QAFkC9amMove4mrLtgwOGimXCWDiwfa9BtASeRNXfqU=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=CdkjjgEI/wgMbLXRYm041Ood9Coxkcz6JKX2GD2FwKo4QT/AISX56cCdkYfcsxtGz xJM2FfrKeBW1S9OAUreyyQpVk9WJW4LB/OkURoYVuFCfr27TbVFxKSIhK4lAsC0eSs sm/84b31OD25g1kdRH+0zkWOldxsCJesr6nZm//mwU+gHgCCWyZ8r1IrVPSBZxknFi ObL/YyySzdB9uUcZ+tl4F+6TeqUP8YP60HePn7UcJtuW4jn5mGmqgUDkRSRzXbdaV+ aNi/8ZCkIGJf0nGAobl2puNw5zG8n55+gkiaaTC9E0Nnwds61OjVqjgYvN5qEdIocO ycwW+1wNY1W5w== Received: from u-boot.org (unknown [73.34.74.121]) by mail.u-boot.org (Postfix) with ESMTPSA id 685F46884F; Tue, 16 Dec 2025 19:29:44 -0700 (MST) From: Simon Glass To: U-Boot Concept Date: Tue, 16 Dec 2025 19:28:04 -0700 Message-ID: <20251217022823.392557-16-sjg@u-boot.org> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20251217022823.392557-1-sjg@u-boot.org> References: <20251217022823.392557-1-sjg@u-boot.org> MIME-Version: 1.0 Message-ID-Hash: MZJUVDUWJTSAJKUK7BQWN76HSLXXJ22X X-Message-ID-Hash: MZJUVDUWJTSAJKUK7BQWN76HSLXXJ22X 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 Opus 4 . 5" X-Mailman-Version: 3.3.10 Precedence: list Subject: [Concept] [PATCH 15/24] pickman: Add database tracking for comment processing 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 Add schema v3 with a processed_comment table to track which MR comments have been addressed by the review agent. This prevents re-processing the same comments when running review or poll commands. Co-developed-by: Claude Opus 4.5 Signed-off-by: Simon Glass --- tools/pickman/README.rst | 11 ++++++ tools/pickman/database.py | 59 +++++++++++++++++++++++++++- tools/pickman/ftest.py | 82 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 1 deletion(-) diff --git a/tools/pickman/README.rst b/tools/pickman/README.rst index a382c98eac0..a9d6809d074 100644 --- a/tools/pickman/README.rst +++ b/tools/pickman/README.rst @@ -222,6 +222,17 @@ Tables - ``url``: URL to the merge request - ``created_at``: Timestamp when the MR was created +**comment** + Tracks MR comments that have been processed by the review agent. + + - ``id``: Primary key + - ``mr_iid``: GitLab merge request IID + - ``comment_id``: GitLab comment/note ID + - ``processed_at``: Timestamp when the comment was processed + + This table prevents the same comment from being addressed multiple times + when running ``review`` or ``poll`` commands. + Configuration ------------- diff --git a/tools/pickman/database.py b/tools/pickman/database.py index c8ed8a6df09..b8da21caf58 100644 --- a/tools/pickman/database.py +++ b/tools/pickman/database.py @@ -11,6 +11,7 @@ To adjust the schema, increment LATEST, create a _migrate_to_v() function and add code in migrate_to() to call it. """ +from datetime import datetime import os import sqlite3 @@ -18,7 +19,7 @@ from u_boot_pylib import tools from u_boot_pylib import tout # Schema version (version 0 means there is no database yet) -LATEST = 2 +LATEST = 3 # Default database filename DB_FNAME = '.pickman.db' @@ -129,6 +130,17 @@ class Database: # pylint: disable=too-many-public-methods 'created_at TEXT, ' 'FOREIGN KEY (source_id) REFERENCES source(id))') + def _create_v3(self): + """Migrate database to v3 schema - add comment table""" + # Table for tracking processed MR comments + self.cur.execute( + 'CREATE TABLE comment (' + 'id INTEGER PRIMARY KEY AUTOINCREMENT, ' + 'mr_iid INTEGER, ' + 'comment_id INTEGER, ' + 'processed_at TEXT, ' + 'UNIQUE(mr_iid, comment_id))') + def migrate_to(self, dest_version): """Migrate the database to the selected version @@ -151,6 +163,8 @@ class Database: # pylint: disable=too-many-public-methods self._create_v1() elif version == 2: self._create_v2() + elif version == 3: + self._create_v3() self.cur.execute('DELETE FROM schema_version') self.cur.execute( @@ -413,3 +427,46 @@ class Database: # pylint: disable=too-many-public-methods """ self.execute( 'UPDATE mergereq SET status = ? WHERE mr_id = ?', (status, mr_id)) + + # comment functions + + def comment_is_processed(self, mr_iid, comment_id): + """Check if a comment has been processed + + Args: + mr_iid (int): Merge request IID + comment_id (int): Comment ID + + Return: + bool: True if already processed + """ + res = self.execute( + 'SELECT id FROM comment WHERE mr_iid = ? AND comment_id = ?', + (mr_iid, comment_id)) + return res.fetchone() is not None + + def comment_mark_processed(self, mr_iid, comment_id): + """Mark a comment as processed + + Args: + mr_iid (int): Merge request IID + comment_id (int): Comment ID + """ + self.execute( + 'INSERT OR IGNORE INTO comment ' + '(mr_iid, comment_id, processed_at) VALUES (?, ?, ?)', + (mr_iid, comment_id, datetime.now().isoformat())) + + def comment_get_processed(self, mr_iid): + """Get all processed comment IDs for an MR + + Args: + mr_iid (int): Merge request IID + + Return: + list: List of comment IDs + """ + res = self.execute( + 'SELECT comment_id FROM comment WHERE mr_iid = ?', + (mr_iid,)) + return [row[0] for row in res.fetchall()] diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index ab4cceccac7..ae4ceaceaaa 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -721,6 +721,88 @@ class TestDatabaseCommitMergereq(unittest.TestCase): dbs.close() +class TestDatabaseComment(unittest.TestCase): + """Tests for Database comment functions.""" + + def setUp(self): + """Set up test fixtures.""" + fd, self.db_path = tempfile.mkstemp(suffix='.db') + os.close(fd) + os.unlink(self.db_path) + + def tearDown(self): + """Clean up test fixtures.""" + if os.path.exists(self.db_path): + os.unlink(self.db_path) + database.Database.instances.clear() + + def test_comment_mark_and_check_processed(self): + """Test marking and checking processed comments""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + + # Comment should not be processed initially + self.assertFalse(dbs.comment_is_processed(123, 456)) + + # Mark as processed + dbs.comment_mark_processed(123, 456) + dbs.commit() + + # Now should be processed + self.assertTrue(dbs.comment_is_processed(123, 456)) + + # Different comment should not be processed + self.assertFalse(dbs.comment_is_processed(123, 789)) + self.assertFalse(dbs.comment_is_processed(999, 456)) + + dbs.close() + + def test_comment_get_processed(self): + """Test getting all processed comments for an MR""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + + # Mark several comments as processed + dbs.comment_mark_processed(100, 1) + dbs.comment_mark_processed(100, 2) + dbs.comment_mark_processed(100, 3) + dbs.comment_mark_processed(200, 10) # Different MR + dbs.commit() + + # Get processed for MR 100 + processed = dbs.comment_get_processed(100) + self.assertEqual(len(processed), 3) + self.assertIn(1, processed) + self.assertIn(2, processed) + self.assertIn(3, processed) + self.assertNotIn(10, processed) + + # Get processed for MR 200 + processed = dbs.comment_get_processed(200) + self.assertEqual(len(processed), 1) + self.assertIn(10, processed) + + dbs.close() + + def test_comment_mark_processed_idempotent(self): + """Test that marking same comment twice doesn't fail""" + with terminal.capture(): + dbs = database.Database(self.db_path) + dbs.start() + + # Mark same comment twice (should not raise) + dbs.comment_mark_processed(123, 456) + dbs.comment_mark_processed(123, 456) + dbs.commit() + + # Should still be processed + self.assertTrue(dbs.comment_is_processed(123, 456)) + + dbs.close() + + class TestListSources(unittest.TestCase): """Tests for list-sources command."""