fix: derive a destination id from its address, not its position

A positional id names a slot in one run's list, and report may run again on
a case whose contacts have changed since. A newly resolved indicator whose
desk sorts ahead renumbers every desk after it, so bodies/<id>.xarf on disk
comes to belong to a different desk than the manifest entry sharing that id.
The body hash recorded per destination would then compare one desk's body
against another's, reporting an edit nobody made or missing one that was.

Hashing the address makes an id follow the desk. It hashes the same
normalised form the grouping uses, so two spellings of one desk share an id
rather than letting whichever spelling RDAP published first decide a body's
filename. Eight hex chars is a deliberate ceiling: collision probability is
about 1e-8 at ten desks, and a short id keeps a case directory readable to
the person reviewing it.

Also strengthens two tests that passed against trivially wrong code: the
no-address case now asserts alongside a contact that does resolve, so
returning nothing at all is no longer a passing answer, and the two-address
case asserts the ioc lists and distinct ids rather than the sorted targets
alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xj1ayFRSUQ2u7cwb3S4axE

2 files changed, 132 insertions, 33 deletions

diff --git a/abusectl/report.py b/abusectl/report.py
index cf839f3..06a3cb5 100644
--- a/abusectl/report.py
+++ b/abusectl/report.py
@@ -26,6 +26,8 @@ thing in a report disclosed deliberately, and a module that reaches for it
 itself is a module that can disclose it in a code path nobody reviewed.
 """
 
+import hashlib
+
 
 def _group_key(address: str) -> str:
     """The key two spellings of one desk must share, and no more than that.
@@ -52,6 +54,32 @@ def _group_key(address: str) -> str:
     return f"{local}@{domain.lower()}"
 
 
+def email_destination_id(address: str) -> str:
+    """The id of the destination that reports to this desk.
+
+    Derived from the ADDRESS, so an id names a desk rather than a position
+    in whatever list this run happened to build.
+
+    That distinction is the whole point. `report` may run again on a case,
+    and `contacts` may have resolved a new indicator since; a positional id
+    then renumbers every desk after the newcomer. Bodies are written to
+    bodies/<id>.xarf and each body's SHA-256 is recorded against its id, so
+    after a renumber the file on disk belongs to a DIFFERENT desk than the
+    manifest entry sharing its id. The edit check would compare one desk's
+    body against another's, reporting an edit nobody made, or, if the two
+    happened to match, missing one that was.
+
+    It hashes the same normalised form the grouping uses, so two spellings
+    of one desk get one id. Deriving it from the raw text instead would let
+    the spelling RDAP published first decide a body's filename.
+    """
+    digest = hashlib.sha256(_group_key(address).encode("utf-8")).hexdigest()
+    # ponytail: 8 hex chars, a case has a handful of desks; widen if a
+    # collision is ever observed. A short id keeps a case directory
+    # readable to the human reviewing it, which is what it is for.
+    return f"email-{digest[:8]}"
+
+
 def email_destinations(contacts: list[dict]) -> list[dict]:
     """Group contacts into one destination per abuse ADDRESS.
 
@@ -60,18 +88,15 @@ def email_destinations(contacts: list[dict]) -> list[dict]:
     common case. One mail per address rather than per contact is what stops
     a desk receiving two mails about one incident.
 
-    Ids are numbered over the DESTINATIONS produced, not over the contacts
-    read, so a contact that resolved to no desk leaves no hole: a reviewer
-    who sees "email-2" and "email-4" reasonably reads two reports as
-    missing.
-
-    Insertion order carries the numbering, so the same contacts in the same
-    order always produce the same ids. That matters downstream: bodies are
-    written to bodies/<id>.xarf and hashed against that id, so an id that
-    wandered between two runs over one input would compare one desk's body
-    against another's. It is NOT a promise that an id survives a change in
-    the contacts themselves; adding a desk earlier in the list renumbers
-    every desk after it.
+    Each id is derived from its own target address, so it survives both a
+    reordering and a change in the contacts: a desk keeps its id when a new
+    indicator resolves to a new desk ahead of it. See email_destination_id
+    for why that matters more than it first appears.
+
+    The returned ORDER is still first-seen, because the destinations are a
+    list a human reads during review and the order the indicators were
+    found in is the most explicable one available. Nothing downstream may
+    key off that order; the id is what identifies a destination.
     """
     by_address: dict[str, dict] = {}
 
@@ -88,12 +113,12 @@ def email_destinations(contacts: list[dict]) -> list[dict]:
 
     return [
         {
-            "id": f"email-{index}",
+            "id": email_destination_id(destination["target"]),
             "kind": "email",
             "target": destination["target"],
             "iocs": destination["iocs"],
             "body": None,
             "status": "pending",
         }
-        for index, destination in enumerate(by_address.values(), start=1)
+        for destination in by_address.values()
     ]
diff --git a/tests/test_report.py b/tests/test_report.py
index 6a0dc51..c72974e 100644
--- a/tests/test_report.py
+++ b/tests/test_report.py
@@ -22,17 +22,36 @@ class Grouping(unittest.TestCase):
              "abuse": ["a@host.invalid", "b@host.invalid"], "source": "rdap"},
         ]
         destinations = report.email_destinations(contacts)
-        self.assertEqual(
-            sorted(d["target"] for d in destinations),
-            ["a@host.invalid", "b@host.invalid"],
-        )
+        self.assertEqual([d["target"] for d in destinations],
+                         ["a@host.invalid", "b@host.invalid"])
+        # Both desks carry the indicator, and each gets its own id: a
+        # destination that reached only one desk, or two rows sharing an
+        # id, would pass an assertion on the sorted targets alone.
+        self.assertEqual([d["iocs"] for d in destinations],
+                         [["ioc-1"], ["ioc-1"]])
+        self.assertEqual(len({d["id"] for d in destinations}), 2)
+        for destination in destinations:
+            self.assertEqual(destination["id"],
+                             report.email_destination_id(
+                                 destination["target"]))
 
     def test_a_contact_with_no_address_creates_no_destination(self):
+        """The contact that resolved must still produce its destination.
+
+        Asserted alongside one that DOES resolve, because "no destination
+        for this contact" is also what returning nothing at all looks
+        like, and that is not the behaviour being described.
+        """
         contacts = [
             {"iocs": ["ioc-1"], "query": "example.invalid", "abuse": [],
              "source": "rdap", "error": "no abuse role published"},
+            {"iocs": ["ioc-2"], "query": "198.51.100.7",
+             "abuse": ["abuse@host.invalid"], "source": "rdap"},
         ]
-        self.assertEqual(report.email_destinations(contacts), [])
+        destinations = report.email_destinations(contacts)
+        self.assertEqual([d["target"] for d in destinations],
+                         ["abuse@host.invalid"])
+        self.assertEqual(destinations[0]["iocs"], ["ioc-2"])
 
     def test_destinations_carry_stable_ids_and_pending_status(self):
         contacts = [
@@ -40,17 +59,26 @@ class Grouping(unittest.TestCase):
              "abuse": ["abuse@host.invalid"], "source": "rdap"},
         ]
         destination = report.email_destinations(contacts)[0]
-        self.assertEqual(destination["id"], "email-1")
+        self.assertEqual(destination["id"],
+                         report.email_destination_id("abuse@host.invalid"))
         self.assertEqual(destination["kind"], "email")
         self.assertEqual(destination["status"], "pending")
 
-    def test_ids_are_numbered_per_destination_not_per_contact(self):
-        """A skipped contact must not leave a hole in the numbering.
+    def test_an_id_is_the_literal_shape_a_reviewer_will_read(self):
+        """Pin the shape, since it becomes a filename in bodies/.
+
+        Computed by hand rather than by calling the code under test, so
+        this fails if the derivation changes rather than following it.
+        """
+        self.assertEqual(report.email_destination_id("abuse@host.invalid"),
+                         "email-bc50e369")
+
+    def test_ids_are_derived_per_destination_not_per_contact(self):
+        """A contact that resolved to no desk must not shift another's id.
 
-        The obvious implementation enumerates the contacts, and a contact
-        with no abuse address then burns an id: the desks come back as
-        "email-2" and "email-4", which reads to a reviewer as two reports
-        having gone missing.
+        The obvious implementation numbers destinations by position, and a
+        skipped contact then either burns an id or renumbers the rest.
+        Both are wrong for the same reason: an id names a desk.
         """
         contacts = [
             {"iocs": ["ioc-1"], "query": "example.invalid", "abuse": [],
@@ -62,7 +90,8 @@ class Grouping(unittest.TestCase):
         ]
         destinations = report.email_destinations(contacts)
         self.assertEqual([d["id"] for d in destinations],
-                         ["email-1", "email-2"])
+                         [report.email_destination_id("a@host.invalid"),
+                          report.email_destination_id("b@host.invalid")])
         self.assertEqual([d["target"] for d in destinations],
                          ["a@host.invalid", "b@host.invalid"])
 
@@ -95,6 +124,16 @@ class Grouping(unittest.TestCase):
         self.assertEqual(destinations[0]["iocs"], ["ioc-1", "ioc-2"])
         self.assertEqual(destinations[0]["target"], "abuse@Host.Invalid")
 
+    def test_two_spellings_of_one_desk_share_an_id(self):
+        """The id derives from the same normalised form the grouping uses.
+
+        Otherwise the spelling RDAP happened to publish first would decide
+        a body's filename, and a re-run that saw the other spelling first
+        would look like a different desk.
+        """
+        self.assertEqual(report.email_destination_id("abuse@Host.Invalid"),
+                         report.email_destination_id("abuse@host.invalid"))
+
     def test_two_local_part_cases_stay_two_destinations(self):
         """Only the receiving host knows whether its local parts fold.
 
@@ -109,6 +148,7 @@ class Grouping(unittest.TestCase):
         destinations = report.email_destinations(contacts)
         self.assertEqual(sorted(d["target"] for d in destinations),
                          ["Abuse@host.invalid", "abuse@host.invalid"])
+        self.assertNotEqual(destinations[0]["id"], destinations[1]["id"])
 
     def test_a_destination_starts_with_no_body(self):
         contacts = [
@@ -118,12 +158,7 @@ class Grouping(unittest.TestCase):
         self.assertIsNone(report.email_destinations(contacts)[0]["body"])
 
     def test_the_same_contacts_produce_the_same_ids_twice(self):
-        """Ids must not depend on dict iteration luck or set ordering.
-
-        Task 8 writes each body to bodies/<id>.xarf and records its hash
-        against that id, so an id that moved between two runs over the
-        same input would compare one desk's body against another's.
-        """
+        """Ids must not depend on dict iteration luck or set ordering."""
         contacts = [
             {"iocs": ["ioc-1"], "query": "198.51.100.7",
              "abuse": ["b@host.invalid", "a@host.invalid"], "source": "rdap"},
@@ -138,6 +173,45 @@ class Grouping(unittest.TestCase):
                          ["b@host.invalid", "a@host.invalid",
                           "c@host.invalid"])
 
+    def test_a_desks_id_survives_another_desk_appearing(self):
+        """An id names a DESK, not a position in this run's list.
+
+        Task 8 writes each body to bodies/<id>.xarf and records its hash
+        against that id. With a positional id, re-running contacts on a
+        case that gained an indicator renumbers every desk after the new
+        one, so bodies/<id>.xarf on disk belongs to a different desk than
+        the manifest's entry of that id, and the edit check compares one
+        desk's body against another's.
+        """
+        established = {"iocs": ["ioc-1"], "query": "198.51.100.7",
+                       "abuse": ["b@host.invalid"], "source": "rdap"}
+        first = report.email_destinations([established])
+
+        # A later contacts run finds an indicator whose desk sorts ahead.
+        newcomer = {"iocs": ["ioc-2"], "query": "example.invalid",
+                    "abuse": ["a@new.invalid"], "source": "rdap"}
+        second = report.email_destinations([newcomer, established])
+
+        by_target = {d["target"]: d["id"] for d in second}
+        self.assertEqual(by_target["b@host.invalid"], first[0]["id"])
+        self.assertNotEqual(by_target["a@new.invalid"], first[0]["id"])
+
+    def test_an_ids_position_does_not_leak_into_it(self):
+        """The same desk alone and third in a list gets one id."""
+        alone = report.email_destinations([
+            {"iocs": ["ioc-1"], "query": "198.51.100.7",
+             "abuse": ["desk@host.invalid"], "source": "rdap"},
+        ])
+        crowded = report.email_destinations([
+            {"iocs": ["ioc-2"], "query": "198.51.100.8",
+             "abuse": ["one@host.invalid", "two@host.invalid"],
+             "source": "rdap"},
+            {"iocs": ["ioc-1"], "query": "198.51.100.7",
+             "abuse": ["desk@host.invalid"], "source": "rdap"},
+        ])
+        self.assertEqual(crowded[2]["target"], "desk@host.invalid")
+        self.assertEqual(crowded[2]["id"], alone[0]["id"])
+
 
 if __name__ == "__main__":
     unittest.main()