From: Simon Glass <simon.glass@canonical.com> 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 <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- 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<x>() 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.""" -- 2.43.0