Merge remote-tracking branch 'origin/main' into fix/command-exec-fixture-leaks

tests/test_claim_command.py

@@ -7,7 +7,24 @@ from tools.ci import nightly_set as ns
 
 ROOT = Path(__file__).resolve().parents[1]
 NOW = datetime(2026, 10, 29, 12, tzinfo=timezone.utc)
-LIST = ns.read_reviewers("kacpherek\nrdbtCVS\n")
+# Fixture accounts: the decision is by numeric ID, so each test login has one. Ludovic's real ID may not be filled in
+# yet (tools/ci/resolve_reviewer_ids.py), so tests use their own unless it is.
+LUDOVIC_ID = ns.OWNER_ID or 1
+ns.OWNER_ID = LUDOVIC_ID
+cc.ns.OWNER_ID = LUDOVIC_ID  # the script's own import of nightly_set
+IDS = {"ludovic111": LUDOVIC_ID, "kacpherek": 101, "rdbtcvs": 102, "levzzz5154": 104}
+
+
+def actor(login):
+    return ns.Actor(login, IDS.get(login.lower(), 9999))
+
+
+def listing(*lines):
+    """A reviewers file: `kacpherek` becomes `101 kacpherek`; other lines stay as written."""
+    return ns.read_reviewers("".join((f"{IDS[l.lower()]} {l}" if l.lower() in IDS else l) + "\n" for l in lines))
+
+
+LIST = listing("kacpherek", "rdbtCVS")
 
 
 def thread(state="open", holders=(), pr=False, locked=False):
@@ -41,7 +58,7 @@ class Fake:
 
 def do(text, api=None, login="newcomer", reviewers=LIST, n=5):
     api = api or Fake()
-    return cc.run(api, cc.parse(text), login, n, reviewers), api
+    return cc.run(api, cc.parse(text), actor(login), n, reviewers), api
 
 
 class ParseTests(unittest.TestCase):
@@ -116,7 +133,7 @@ class ClaimTests(unittest.TestCase):
         do("/claim", Fake(claims=50), login="ludovic111")
         do("/claim", Fake(claims=50), login="RdbtCVS")
         with self.assertRaises(cc.Refused):
-            do("/claim", Fake(claims=50), login="rdbtCVS", reviewers=ns.read_reviewers("rdbtCVS\npaused\n"))
+            do("/claim", Fake(claims=50), login="rdbtCVS", reviewers=listing("rdbtCVS", "paused"))
 
     def test_github_dropped_the_assignee(self):
         with self.assertRaisesRegex(cc.Refused, "didn't assign"):

tests/test_nightly.py

@@ -19,7 +19,8 @@ def lookup(*prs):
 
 
 def run(state, text, login="kacpherek", *prs):
-    return ns.apply(state, ns.parse(text), login, NOW, lookup(*prs), PATTERNS)
+    return ns.apply(state, ns.parse(text), ns.Actor(login, 101 if login == "kacpherek" else 9999), NOW,
+                    lookup(*prs), PATTERNS)
 
 T = dt.datetime(2026, 10, 28, 12, 0, tzinfo=dt.timezone.utc)
 

tests/test_nightly_set.py

@@ -10,6 +10,20 @@ from tools.ci import nightly_set as ns
 ROOT = Path(__file__).resolve().parents[1]
 NOW = "2026-10-28T12:00:00+00:00"
 PATTERNS = ns.guarded_patterns((ROOT / ".github/CODEOWNERS").read_text())
+# Fixture accounts: the decision is by numeric ID, so each test login has one. Ludovic's real ID may not be filled in
+# yet (tools/ci/resolve_reviewer_ids.py), so tests use their own unless it is.
+LUDOVIC_ID = ns.OWNER_ID or 1
+ns.OWNER_ID = LUDOVIC_ID
+IDS = {"ludovic111": LUDOVIC_ID, "kacpherek": 101, "rdbtcvs": 102, "levzzz5154": 104}
+
+
+def actor(login):
+    return ns.Actor(login, IDS.get(login.lower(), 9999))
+
+
+def listing(*lines):
+    """A reviewers file: `kacpherek` becomes `101 kacpherek`; other lines stay as written."""
+    return ns.read_reviewers("".join((f"{IDS[l.lower()]} {l}" if l.lower() in IDS else l) + "\n" for l in lines))
 
 
 def pr(n, head="h", state="open", files=("crates/horizon-world/src/lib.rs",), base="main"):
@@ -22,7 +36,7 @@ def lookup(*prs):
 
 
 def run(state, text, login="kacpherek", *prs):
-    return ns.apply(state, ns.parse(text), login, NOW, lookup(*prs), PATTERNS)
+    return ns.apply(state, ns.parse(text), actor(login), NOW, lookup(*prs), PATTERNS)
 
 
 class CommandTests(unittest.TestCase):
@@ -30,8 +44,8 @@ class CommandTests(unittest.TestCase):
 
     def run_command(self, body, author=ns.OWNER):
         answers = []
-        env = {"GITHUB_REPOSITORY": "horizon-rust/horizon", "ISSUE": "1", "AUTHOR": author, "BODY": body,
-               "COMMENT_URL": "", "GH_TOKEN": "token"}
+        env = {"GITHUB_REPOSITORY": "horizon-rust/horizon", "ISSUE": "1", "AUTHOR": author,
+               "AUTHOR_ID": str(IDS.get(author.lower(), 9999)), "BODY": body, "COMMENT_URL": "", "GH_TOKEN": "token"}
         with mock.patch.dict(os.environ, env), \
                 mock.patch.object(ns, "load_state", return_value=({"prs": [], "blocked": []}, "", None)), \
                 mock.patch.object(ns, "lookup", return_value=lookup()), \
@@ -67,9 +81,9 @@ class NoReviewNeededTests(unittest.TestCase):
         with self.assertRaisesRegex(ns.Refused, "is closed"):
             run({}, "/nightly add 1", "kacpherek", pr(1, state="closed"))
         with self.assertRaises(ns.Refused):  # who may ask is checked before the set changes (command())
-            ns.authorize("someone", ns.read_reviewers("kacpherek\n"))
+            ns.authorize(actor("someone"), listing("kacpherek"))
         with self.assertRaises(ns.Refused):
-            ns.authorize("kacpherek", ns.read_reviewers("kacpherek\npaused\n"))
+            ns.authorize(actor("kacpherek"), listing("kacpherek", "paused"))
 
 
 class ParseTests(unittest.TestCase):
@@ -101,30 +115,74 @@ class ParseTests(unittest.TestCase):
 
 
 class AllowTests(unittest.TestCase):
-    LIST = "# trusted\nkacpherek   # added by Ludovic\nrdbtCVS\n\nnot a login!\n"
+    LIST = "# trusted\n101 kacpherek   # added by Ludovic\n102 rdbtCVS\n\nnot a login!\n"
 
     def test_listed_reviewers_and_ludovic(self):
         reviewers = ns.read_reviewers(self.LIST)
-        ns.authorize("kacpherek", reviewers)
-        ns.authorize("RDBTcvs", reviewers)
-        ns.authorize("ludovic111", reviewers)
+        ns.authorize(actor("kacpherek"), reviewers)
+        ns.authorize(actor("RDBTcvs"), reviewers)
+        ns.authorize(actor("ludovic111"), reviewers)
         for login in ("levzzz5154", "someone", "not"):
             with self.assertRaises(ns.Refused):
-                ns.authorize(login, reviewers)
+                ns.authorize(actor(login), reviewers)
+
+    def test_the_id_decides_not_the_login(self):
+        reviewers = ns.read_reviewers(self.LIST)
+        # Someone who claimed a listed login after a rename: same name, another account.
+        with self.assertRaises(ns.Refused):
+            ns.authorize(ns.Actor("kacpherek", 555), reviewers)
+        with self.assertRaises(ns.Refused):
+            ns.authorize(ns.Actor("ludovic111", 556), reviewers)
+        # The listed account under a new name keeps its access.
+        ns.authorize(ns.Actor("kacper-renamed", 101), reviewers)
+        ns.authorize(ns.Actor("new-name", LUDOVIC_ID), reviewers)
+
+    def test_old_format_lines_grant_nothing(self):
+        reviewers = ns.read_reviewers("kacpherek\n101\n101 kacpherek extra\n0 zero\n")
+        self.assertEqual(reviewers.members, {})
+        self.assertEqual(len(reviewers.skipped), 4)
+        with self.assertRaises(ns.Refused):
+            ns.authorize(actor("kacpherek"), reviewers)
+
+    def test_no_owner_id_means_no_owner(self):
+        saved = ns.OWNER_ID
+        try:
+            ns.OWNER_ID = None
+            self.assertFalse(ns.is_owner(ns.Actor("ludovic111", LUDOVIC_ID)))
+            self.assertFalse(ns.is_owner_id(None))
+        finally:
+            ns.OWNER_ID = saved
 
     def test_paused_stops_reviewers_not_ludovic(self):
         reviewers = ns.read_reviewers(self.LIST + "paused  # Ludovic, 2026-10-29\n")
         self.assertTrue(reviewers.paused)
         with self.assertRaises(ns.Refused):
-            ns.authorize("kacpherek", reviewers)
-        ns.authorize("ludovic111", reviewers)
+            ns.authorize(actor("kacpherek"), reviewers)
+        ns.authorize(actor("ludovic111"), reviewers)
 
-    def test_the_repos_list_names_its_reviewers(self):
+    def test_the_author_id_comes_from_the_workflow(self):
+        self.assertEqual(ns.actor_from_env({"AUTHOR": "kacpherek", "AUTHOR_ID": "101"}), ns.Actor("kacpherek", 101))
+        for env in ({"AUTHOR": "kacpherek"}, {"AUTHOR": "kacpherek", "AUTHOR_ID": ""},
+                    {"AUTHOR": "kacpherek", "AUTHOR_ID": "kacpherek"}, {"AUTHOR": "x", "AUTHOR_ID": "-1"}):
+            with self.assertRaises(ns.Refused):
+                ns.actor_from_env(env)
+
+    def test_the_repos_list_names_its_reviewers_by_id(self):
+        """Fails until tools/ci/resolve_reviewer_ids.py has filled in the IDs: until then no reviewer has access."""
         reviewers = ns.read_reviewers((ROOT / ns.REVIEWERS).read_text())
-        self.assertEqual(
-            reviewers.logins, {"kacpherek", "rdbtcvs", "online5277", "levzzz5154", "qwerner88880"}
-        )
+        self.assertEqual(reviewers.skipped, [])
+        self.assertEqual(reviewers.logins, {"kacpherek", "rdbtcvs", "online5277", "levzzz5154", "qwerner88880"})
         self.assertFalse(reviewers.paused)
+        source = (ROOT / "tools/ci/nightly_set.py").read_text()
+        self.assertRegex(source, r"(?m)^OWNER_ID: int \| None = [1-9][0-9]*$")
+
+
+class WorkflowIdTests(unittest.TestCase):
+    def test_every_command_workflow_passes_the_authors_id(self):
+        for name in ("nightly-command.yml", "triage-command.yml", "claim-command.yml"):
+            text = (ROOT / ".github/workflows" / name).read_text()
+            self.assertIn("AUTHOR_ID: ${{ github.event.comment.user.id }}", text, name)
+
 
 class RefusalTests(unittest.TestCase):
     def test_what_codeowners_guards_is_refused(self):

tests/test_triage_command.py

@@ -8,11 +8,28 @@ from tools.ci import triage_command as tc
 ROOT = Path(__file__).resolve().parents[1]
 NOW = datetime(2026, 10, 28, 12, tzinfo=timezone.utc)
 URL = "https://github.com/horizon-rust/horizon/issues/5#issuecomment-1"
-LIST = ns.read_reviewers("kacpherek\nrdbtCVS\n")
+# Fixture accounts: the decision is by numeric ID, so each test login has one. Ludovic's real ID may not be filled in
+# yet (tools/ci/resolve_reviewer_ids.py), so tests use their own unless it is.
+LUDOVIC_ID = ns.OWNER_ID or 1
+ns.OWNER_ID = LUDOVIC_ID
+tc.ns.OWNER_ID = LUDOVIC_ID  # the script's own import of nightly_set
+IDS = {"ludovic111": LUDOVIC_ID, "kacpherek": 101, "rdbtcvs": 102, "levzzz5154": 104}
+
+
+def actor(login):
+    return ns.Actor(login, IDS.get(login.lower(), 9999))
+
+
+def listing(*lines):
+    """A reviewers file: `kacpherek` becomes `101 kacpherek`; other lines stay as written."""
+    return ns.read_reviewers("".join((f"{IDS[l.lower()]} {l}" if l.lower() in IDS else l) + "\n" for l in lines))
+
+
+LIST = listing("kacpherek", "rdbtCVS")
 
 
 def thread(state="open", user="dev", labels=("bug",), pr=False, merged=False, locked=False):
-    t = {"state": state, "user": {"login": user}, "labels": [{"name": n} for n in labels], "locked": locked}
+    t = {"state": state, "user": {"login": user, "id": IDS.get(user.lower(), 9999)}, "labels": [{"name": n} for n in labels], "locked": locked}
     if pr:
         t["pull_request"] = {"merged_at": "2026-10-27T00:00:00Z" if merged else None}
     return t
@@ -50,7 +67,7 @@ class Fake:
 
 def do(text, api=None, login="kacpherek", reviewers=LIST, n=5):
     api = api or Fake()
-    return tc.run(api, tc.parse(text), login, n, URL, reviewers, NOW), api
+    return tc.run(api, tc.parse(text), actor(login), n, URL, reviewers, NOW), api
 
 
 class ParseTests(unittest.TestCase):
@@ -96,16 +113,21 @@ class AllowTests(unittest.TestCase):
             do("/close", login="someone")
 
     def test_paused(self):
-        paused = ns.read_reviewers("kacpherek\npaused\n")
+        paused = listing("kacpherek", "paused")
         with self.assertRaisesRegex(tc.Refused, "paused"):
             do("/close", reviewers=paused)
         do("/close", reviewers=paused, login="ludovic111")
 
     def test_the_repos_list_is_the_nightlys(self):
         self.assertEqual(ns.REVIEWERS, ".github/trusted-reviewers.txt")
-        listed = ns.read_reviewers((ROOT / ns.REVIEWERS).read_text())
-        self.assertEqual(listed.logins, {"kacpherek", "rdbtcvs", "online5277", "levzzz5154", "qwerner88880"})
-        self.assertFalse(listed.paused)
+        self.assertEqual(Path(tc.ns.__file__).resolve(), Path(ns.__file__).resolve())  # one parser for all three
+
+    def test_a_reclaimed_login_is_nobody(self):
+        with self.assertRaisesRegex(tc.Refused, "Only Ludovic"):
+            tc.run(Fake(), tc.parse("/close"), ns.Actor("kacpherek", 555), 5, URL, LIST, NOW)
+        # Ludovic's threads are recognised by his ID, not by a login someone else may hold.
+        api = Fake({5: {"state": "open", "user": {"login": "ludovic111", "id": 555}, "labels": [], "locked": False}})
+        do("/close", api)
 
 
 class ActionTests(unittest.TestCase):