PR comments forwarding

kernel_patches_daemon/branch_worker.py

@@ -21,7 +21,7 @@
 from collections import namedtuple
 from contextlib import contextmanager
 from datetime import datetime, timedelta, timezone
-from email.mime.application import MIMEApplication
+from email.message import EmailMessage
 from email.mime.multipart import MIMEMultipart
 from email.mime.text import MIMEText
 from enum import Enum
@@ -248,28 +248,28 @@ def bump_email_status_counters(status: Status):
 
 def generate_msg_id(host: str) -> str:
     """Generate an email message ID based on the provided host."""
+    # RFC 822 style message ID to allow for easier referencing of this
+    # message. Note that it's not entirely correct for us to refer to a host
+    # that is not entirely under our control, but we don't want to expose our
+    # actual host name either. Collisions of a sha256 hash are assumed to be
+    # unlikely in many contexts, so we do the same.
     checksum = hashlib.sha256(str(time.time()).encode("utf-8")).hexdigest()
     return f"{checksum}@{host}"
 
 
-def email_in_submitter_allowlist(email: str, allowlist: Sequence[re.Pattern]) -> bool:
-    """
-    Checks if an email is in the submitter allowlist
-
-    Note that there may be false positives when folks have regex syntax in
-    their email address. But that is ok -- this is simply a rollout mechanism.
-    We only need to roughly control the rollout.
-    """
-    return any(regex.fullmatch(email) for regex in allowlist)
+def email_matches_any(email: str, patterns: Sequence[re.Pattern]) -> bool:
+    return any(regex.fullmatch(email) for regex in patterns)
 
 
 def build_email(
     config: EmailConfig,
-    series: Series,
+    to_list: List[str],
+    cc_list: List[str],
     subject: str,
     msg_id: str,
     body: str,
-    boundary: str = "",
+    boundary: Optional[str] = None,
+    in_reply_to: Optional[str] = None,
 ) -> Tuple[List[str], str]:
     """
     Builds complete email (including headers) to be sent along with curl command
@@ -297,36 +297,24 @@ def build_email(
         "-",
     ]
 
-    to_list = copy.copy(config.smtp_to)
-    cc_list = copy.copy(config.smtp_cc)
-
-    if config.ignore_allowlist or email_in_submitter_allowlist(
-        series.submitter_email, config.submitter_allowlist
-    ):
-        to_list += [series.submitter_email]
-
     for to in to_list + cc_list:
         args += ["--mail-rcpt", to]
 
     if config.smtp_http_proxy is not None:
         args += ["--proxy", config.smtp_http_proxy]
 
     msg = MIMEMultipart()
-    # Add some RFC 822 style message ID to allow for easier referencing of this
-    # message. Note that it's not entirely correct for us to refer to a host
-    # that is not entirely under our control, but we don't want to expose our
-    # actual host name either. Collisions of a sha256 hash are assumed to be
-    # unlikely in many contexts, so we do the same.
     msg["Message-Id"] = f"<{msg_id}>"
-    msg["In-Reply-To"] = get_ci_base(series)["msgid"]
+    if in_reply_to is not None:
+        msg["In-Reply-To"] = in_reply_to
     msg["References"] = msg["In-Reply-To"]
     msg["Subject"] = subject
     msg["From"] = config.smtp_from
     if to_list:
         msg["To"] = ",".join(to_list)
     if cc_list:
         msg["Cc"] = ",".join(cc_list)
-    if boundary:
+    if boundary is not None:
         msg.set_boundary(boundary)
     msg.attach(MIMEText(body, "plain"))
 
@@ -335,14 +323,17 @@ def build_email(
 
 async def send_email(
     config: EmailConfig,
-    series: Series,
+    to_list: List[str],
+    cc_list: List[str],
     subject: str,
     body: str,
-):
+    in_reply_to: Optional[str] = None,
+) -> str:
     """Send an email."""
     msg_id = generate_msg_id(config.smtp_host)
-    curl_args, msg = build_email(config, series, subject, msg_id, body)
-
+    curl_args, msg = build_email(
+        config, to_list, cc_list, subject, msg_id, body, in_reply_to=in_reply_to
+    )
     proc = await asyncio.create_subprocess_exec(
         *curl_args, stdin=PIPE, stdout=PIPE, stderr=PIPE
     )
@@ -352,6 +343,100 @@ async def send_email(
         logger.error(f"failed to send email: {stdout.decode()} {stderr.decode()}")
         email_send_fail_counter.add(1)
 
+    return msg_id
+
+
+def ci_results_email_recipients(
+    config: EmailConfig, series: Series
+) -> Tuple[List[str], List[str]]:
+    to_list = copy.copy(config.smtp_to)
+    cc_list = copy.copy(config.smtp_cc)
+    if config.ignore_allowlist or email_matches_any(
+        series.submitter_email, config.submitter_allowlist
+    ):
+        to_list += [series.submitter_email]
+
+    return (to_list, cc_list)
+
+
+async def send_ci_results_email(
+    config: EmailConfig,
+    series: Series,
+    subject: str,
+    body: str,
+):
+    (to_list, cc_list) = ci_results_email_recipients(config, series)
+    in_reply_to = get_ci_base(series)["msgid"]
+    await send_email(config, to_list, cc_list, subject, body, in_reply_to)
+
+
+def reply_email_recipients(
+    msg: EmailMessage,
+    allowlist: Optional[Sequence[re.Pattern]] = None,
+    denylist: Optional[Sequence[re.Pattern]] = None,
+) -> Tuple[List[str], List[str]]:
+    """
+    Extracts response recipients from the `msg`, applying allowlist/denylist.
+
+    Args:
+        msg: the EmailMessage we will be replying to
+        allowlist: list of email address regexes to allow, ignored if empty
+        denylist: list of email address regexes to deny, ignored if empty
+
+    Returns:
+        (to_list, cc_list) - recipients of the reply email
+    """
+    tos = msg.get_all("To", [])
+    ccs = msg.get_all("Cc", [])
+    cc_list = [a for (_, a) in email.utils.getaddresses(tos + ccs)]
+
+    (_, sender_address) = email.utils.parseaddr(msg.get("From"))
+    to_list = [sender_address]
+
+    if allowlist:
+        cc_list = [a for a in cc_list if email_matches_any(a, allowlist)]
+        to_list = [a for a in to_list if email_matches_any(a, allowlist)]
+
+    if denylist:
+        cc_list = [a for a in cc_list if not email_matches_any(a, denylist)]
+        to_list = [a for a in to_list if not email_matches_any(a, denylist)]
+
+    return (to_list, cc_list)
+
+
+async def send_pr_comment_email(
+    config: EmailConfig, msg: EmailMessage, body: str
+) -> Optional[str]:
+    """
+    This function forwards a pull request comment (`body`) as an email reply to the original
+    patch email message `msg`. It extracts recipients from the `msg`, applies allowlist, denylist,
+    and always_cc as configured, and sets Subject and In-Reply-To based on the `msg`
+
+    Args:
+        config: EmailConfig with PRCommentsForwardingConfig
+        msg: the original EmailMessage we are replying to (the patch submission)
+        body: the content of the reply we are sending
+
+    Returns:
+        Message-Id of the sent email, or None if it wasn't sent
+    """
+    if config is None or not config.is_pr_comment_forwarding_enabled():
+        return
+
+    cfg = config.pr_comments_forwarding
+    (to_list, cc_list) = reply_email_recipients(
+        msg, allowlist=cfg.recipient_allowlist, denylist=cfg.recipient_denylist
+    )
+    cc_list += cfg.always_cc
+
+    if not to_list and not cc_list:
+        return
+
+    subject = "Re: " + msg.get("Subject")
+    in_reply_to = msg.get("Message-Id")
+
+    return await send_email(config, to_list, cc_list, subject, body, in_reply_to)
+
 
 def pr_has_label(pr: PullRequest, label: str) -> bool:
     for pr_label in pr.get_labels():
@@ -559,7 +644,7 @@ def __init__(
         )
 
         self.patchwork = patchwork
-        self.email = email
+        self.email_config = email
 
         self.log_extractor = log_extractor
         self.ci_repo_url = ci_repo_url
@@ -1204,8 +1289,7 @@ async def evaluate_ci_result(
         self, status: Status, series: Series, pr: PullRequest, jobs: List[WorkflowJob]
     ) -> None:
         """Evaluate the result of a CI run and send an email as necessary."""
-        email = self.email
-        if email is None:
+        if self.email_config is None:
             logger.info("No email configuration present; skipping sending...")
             return
 
@@ -1245,7 +1329,7 @@ async def evaluate_ci_result(
             subject = await get_ci_email_subject(series)
             ctx = build_email_body_context(self.repo, pr, status, series, inline_logs)
             body = furnish_ci_email_body(ctx)
-            await send_email(email, series, subject, body)
+            await send_ci_results_email(self.email_config, series, subject, body)
             bump_email_status_counters(status)
 
     def expire_branches(self) -> None:
@@ -1293,3 +1377,77 @@ async def submit_pr_summary(
             description="PR summary",
         )
         pr_summary_report.add(1)
+
+    async def forward_pr_comments(self, pr: PullRequest, series: Series):
+        if (
+            self.email_config is None
+            or not self.email_config.is_pr_comment_forwarding_enabled()
+        ):
+            return
+        cfg = self.email_config.pr_comments_forwarding
+
+        comments = pr.get_issue_comments()
+
+        # Look for comments indicating what has already been forwarded
+        # to filter them out
+        forwarded_set = set()
+        for comment in comments:
+            match = re.search(
+                r"Forwarding comment \[([0-9]+)\].* via email",
+                comment.body,
+                re.MULTILINE,
+            )
+            if not match:
+                continue
+            forwarded_id = int(match.group(1))
+            forwarded_set.add(forwarded_id)
+
+        for comment in comments:
+            if (
+                comment.user.login not in cfg.commenter_allowlist
+                or comment.id in forwarded_set
+            ):
+                continue
+            # Determine the message to reply to
+            # Check for In-Reply-To-Subject tag in the comment body
+            match = re.search(
+                r"In-Reply-To-Subject: \`(.*)\`", comment.body, re.MULTILINE
+            )
+            if not match:
+                logger.info(
+                    f"Ignoring PR comment {comment.html_url} without In-Reply-To-Subject tag"
+                )
+                continue
+            subject = match.group(1)
+            patch = series.patch_by_subject(subject)
+            if not patch:
+                logger.warn(
+                    f"Ignoring PR comment {comment.html_url}, could not find relevant patch on patchwork"
+                )
+                continue
+
+            # first, post a comment that the message is being forwarded
+            msg_id = patch["msgid"]
+            patch_url = patch["web_url"]
+            message = f"Forwarding comment [{comment.id}]({comment.html_url}) via email"
+            message += f"\nIn-Reply-To: {msg_id}"
+            message += f"\nPatch: {patch_url}"
+            self._add_pull_request_comment(pr, message)
+
+            # then, load the message we are replying to
+            mbox = await series.pw_client.get_blob(patch["mbox"])
+            parser = email.parser.BytesParser(policy=email.policy.default)
+            patch_msg = parser.parsebytes(mbox, headersonly=True)
+
+            # and forward the target comment via email
+            sent_msg_id = await send_pr_comment_email(
+                self.email_config, patch_msg, comment.body
+            )
+            if sent_msg_id is not None:
+                logger.info(
+                    f"Forwarded PR comment {comment.html_url} via email, Message-Id: {sent_msg_id}"
+                )
+            else:
+                logger.warn(
+                    f"Failed to forward PR comment in reply to {msg_id}, no recipients"
+                )

kernel_patches_daemon/config.py

@@ -104,6 +104,29 @@ def from_json(cls, json: Dict) -> "BranchConfig":
         )
 
 
+@dataclass
+class PRCommentsForwardingConfig:
+    enabled: bool
+    always_cc: List[str]
+    commenter_allowlist: List[str]
+    recipient_allowlist: List[re.Pattern]
+    recipient_denylist: List[re.Pattern]
+
+    @classmethod
+    def from_json(cls, json: Dict) -> "PRCommentsForwardingConfig":
+        return cls(
+            enabled=json.get("enabled", False),
+            always_cc=json.get("always_cc", []),
+            recipient_allowlist=[
+                re.compile(pattern) for pattern in json.get("recipient_allowlist", [])
+            ],
+            recipient_denylist=[
+                re.compile(pattern) for pattern in json.get("recipient_denylist", [])
+            ],
+            commenter_allowlist=json.get("commenter_allowlist", []),
+        )
+
+
 @dataclass
 class EmailConfig:
     smtp_host: str
@@ -123,6 +146,7 @@ class EmailConfig:
     # Ignore the `submitter_allowlist` entries and send emails to all patch
     # submitters, unconditionally.
     ignore_allowlist: bool
+    pr_comments_forwarding: Optional[PRCommentsForwardingConfig]
 
     @classmethod
     def from_json(cls, json: Dict) -> "EmailConfig":
@@ -139,8 +163,17 @@ def from_json(cls, json: Dict) -> "EmailConfig":
                 re.compile(pattern) for pattern in json.get("submitter_allowlist", [])
             ],
             ignore_allowlist=json.get("ignore_allowlist", False),
+            pr_comments_forwarding=PRCommentsForwardingConfig.from_json(
+                json.get("pr_comments_forwarding", {})
+            ),
         )
 
+    def is_pr_comment_forwarding_enabled(self) -> bool:
+        if self.pr_comments_forwarding is not None:
+            return self.pr_comments_forwarding.enabled
+        else:
+            return False
+
 
 @dataclass
 class PatchworksConfig:

kernel_patches_daemon/github_sync.py

@@ -269,6 +269,7 @@ async def sync_relevant_subject(self, subject: Subject) -> None:
                 f"Created/updated {pr} ({pr.head.ref}): {pr.url} for series {series.id}"
             )
             await worker.sync_checks(pr, series)
+            await worker.forward_pr_comments(pr, series)
             # Close out other PRs if exists
             self.close_existing_prs_for_series(list(self.workers.values()), pr)
 

kernel_patches_daemon/patchwork.py

@@ -497,6 +497,12 @@ def submitter_email(self):
         """Retrieve the email address of the patch series submitter."""
         return self._submitter_email
 
+    def patch_by_subject(self, subject_str: str) -> Optional[Dict]:
+        for patch in self.patches:
+            if subject_str in patch["name"]:
+                return patch
+        return None
+
 
 class Patchwork:
     def __init__(