Merge pull request #2192 from Urgau/handlers-user-error

Urgau · web-flow · commit 5e11c7bb1e27 · 2025-10-05T13:46:36.000Z
Improve and simplify commands errors
diff --git a/src/github/webhook.rs b/src/github/webhook.rs
@@ -274,11 +274,10 @@ async fn process_payload(
         }
     }
     if !message.is_empty() {
+        log::info!("user error: {}", message);
         if let Some(issue) = event.issue() {
             let cmnt = ErrorComment::new(issue, message);
             cmnt.post(&ctx.github).await?;
-        } else {
-            log::error!("handling event failed: {:?}", message);
         }
     }
     if other_error {
diff --git a/src/handlers.rs b/src/handlers.rs
@@ -10,21 +10,16 @@ use std::fmt;
 use std::sync::Arc;
 use tracing as log;
 
-#[derive(Debug)]
-pub enum HandlerError {
-    Message(String),
-    Other(anyhow::Error),
-}
-
-impl std::error::Error for HandlerError {}
-
-impl fmt::Display for HandlerError {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        match self {
-            HandlerError::Message(msg) => write!(f, "{}", msg),
-            HandlerError::Other(_) => write!(f, "An internal error occurred."),
-        }
-    }
+/// Creates a [`UserError`] with message.
+///
+/// Should be used when an handler is in error due to the user action's (not a PR,
+/// not a issue, not authorized, ...).
+///
+/// Should be used like this `return user_error!("My error message.");`.
+macro_rules! user_error {
+    ($err:expr $(,)?) => {
+        anyhow::Result::Err(anyhow::anyhow!(crate::handlers::UserError($err.into())))
+    };
 }
 
 mod assign;
@@ -61,6 +56,19 @@ mod shortcut;
 mod transfer;
 pub mod types_planning_updates;
 
+pub struct Context {
+    pub github: GithubClient,
+    pub zulip: ZulipClient,
+    pub team: TeamClient,
+    pub db: crate::db::ClientPool,
+    pub username: String,
+    pub octocrab: Octocrab,
+    /// Represents the workqueue (assigned open PRs) of individual reviewers.
+    /// tokio's RwLock is used to avoid deadlocks, since we run on a single-threaded tokio runtime.
+    pub workqueue: Arc<tokio::sync::RwLock<ReviewerWorkqueue>>,
+    pub gha_logs: Arc<tokio::sync::RwLock<GitHubActionLogsCache>>,
+}
+
 pub async fn handle(ctx: &Context, host: &str, event: &Event) -> Vec<HandlerError> {
     let config = config::get(&ctx.github, event.repo()).await;
     if let Err(e) = &config {
@@ -368,11 +376,15 @@ macro_rules! command_handlers {
                         if let Some(config) = &config.$name {
                             $name::handle_command(ctx, config, event, command)
                                 .await
-                                .unwrap_or_else(|err| {
-                                    errors.push(HandlerError::Other(err.context(format!(
-                                        "error when processing {} command handler",
-                                        stringify!($name)
-                                    ))))
+                                .unwrap_or_else(|mut err| {
+                                    if let Some(err) = err.downcast_mut::<UserError>() {
+                                        errors.push(HandlerError::Message(std::mem::take(&mut err.0)));
+                                    } else {
+                                        errors.push(HandlerError::Other(err.context(format!(
+                                            "error when processing {} command handler",
+                                            stringify!($name)
+                                        ))));
+                                    }
                                 });
                         } else {
                             errors.push(HandlerError::Message(format!(
@@ -416,15 +428,33 @@ command_handlers! {
     transfer: Transfer,
 }
 
-pub struct Context {
-    pub github: GithubClient,
-    pub zulip: ZulipClient,
-    pub team: TeamClient,
-    pub db: crate::db::ClientPool,
-    pub username: String,
-    pub octocrab: Octocrab,
-    /// Represents the workqueue (assigned open PRs) of individual reviewers.
-    /// tokio's RwLock is used to avoid deadlocks, since we run on a single-threaded tokio runtime.
-    pub workqueue: Arc<tokio::sync::RwLock<ReviewerWorkqueue>>,
-    pub gha_logs: Arc<tokio::sync::RwLock<GitHubActionLogsCache>>,
+#[derive(Debug)]
+pub enum HandlerError {
+    Message(String),
+    Other(anyhow::Error),
+}
+
+impl std::error::Error for HandlerError {}
+
+impl fmt::Display for HandlerError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            HandlerError::Message(msg) => write!(f, "{}", msg),
+            HandlerError::Other(_) => write!(f, "An internal error occurred."),
+        }
+    }
+}
+
+/// Represent a user error.
+///
+/// The message will be shown to the user via comment posted by this bot.
+#[derive(Debug)]
+pub struct UserError(String);
+
+impl std::error::Error for UserError {}
+
+impl fmt::Display for UserError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.write_str(&self.0)
+    }
 }
diff --git a/src/handlers/assign.rs b/src/handlers/assign.rs
@@ -525,10 +525,7 @@ pub(super) async fn handle_command(
     let issue = event.issue().unwrap();
     if issue.is_pr() {
         if !issue.is_open() {
-            issue
-                .post_comment(&ctx.github, "Assignment is not allowed on a closed PR.")
-                .await?;
-            return Ok(());
+            return user_error!("Assignment is not allowed on a closed PR.");
         }
         if matches!(
             event,
@@ -612,7 +609,7 @@ pub(super) async fn handle_command(
             AssignCommand::Claim => event.user().login.clone(),
             AssignCommand::AssignUser { username } => {
                 if !is_team_member && username != event.user().login {
-                    bail!("Only Rust team members can assign other users");
+                    return user_error!("Only Rust team members can assign other users");
                 }
                 username.clone()
             }
@@ -627,7 +624,7 @@ pub(super) async fn handle_command(
                         e.apply(&ctx.github, String::new()).await?;
                         return Ok(());
                     } else {
-                        bail!("Cannot release another user's assignment");
+                        return user_error!("Cannot release another user's assignment");
                     }
                 } else {
                     let current = &event.user().login;
@@ -639,11 +636,13 @@ pub(super) async fn handle_command(
                         e.apply(&ctx.github, String::new()).await?;
                         return Ok(());
                     } else {
-                        bail!("Cannot release unassigned issue");
+                        return user_error!("Cannot release unassigned issue");
                     }
                 };
             }
-            AssignCommand::RequestReview { .. } => bail!("r? is only allowed on PRs."),
+            AssignCommand::RequestReview { .. } => {
+                return user_error!("r? is only allowed on PRs.");
+            }
         };
         // Don't re-assign if aleady assigned, e.g. on comment edit
         if issue.contain_assignee(&to_assign) {
diff --git a/src/handlers/close.rs b/src/handlers/close.rs
@@ -1,6 +1,6 @@
 //! Allows to close an issue or a PR
 
-use crate::{config::CloseConfig, github::Event, handlers::Context, interactions::ErrorComment};
+use crate::{config::CloseConfig, github::Event, handlers::Context};
 use parser::command::close::CloseCommand;
 
 pub(super) async fn handle_command(
@@ -16,9 +16,7 @@ pub(super) async fn handle_command(
         .await
         .unwrap_or(false);
     if !is_team_member {
-        let cmnt = ErrorComment::new(&issue, "Only team members can close issues.");
-        cmnt.post(&ctx.github).await?;
-        return Ok(());
+        return user_error!("Only team members can close issues.");
     }
     issue.close(&ctx.github).await?;
     Ok(())
diff --git a/src/handlers/concern.rs b/src/handlers/concern.rs
@@ -6,7 +6,7 @@ use crate::{
     config::ConcernConfig,
     github::{Event, Label},
     handlers::Context,
-    interactions::{EditIssueBody, ErrorComment},
+    interactions::EditIssueBody,
 };
 use parser::command::concern::ConcernCommand;
 
@@ -37,7 +37,7 @@ pub(super) async fn handle_command(
     cmd: ConcernCommand,
 ) -> anyhow::Result<()> {
     let Event::IssueComment(issue_comment) = event else {
-        bail!("concern issued on something other than a issue")
+        return user_error!("Concerns can only be issued on an issue");
     };
     let Some(comment_url) = event.html_url() else {
         bail!("unable to retrieve the comment url")
@@ -81,10 +81,9 @@ pub(super) async fn handle_command(
             issue.number,
             issue_comment.comment.user,
         );
-        ErrorComment::new(&issue, "Only team members in the [team repo](https://github.com/rust-lang/team) can add or resolve concerns.")
-            .post(&ctx.github)
-            .await?;
-        return Ok(());
+        return user_error!(
+            "Only team members in the [team repo](https://github.com/rust-lang/team) can add or resolve concerns."
+        );
     }
 
     let mut client = ctx.db.get().await;
diff --git a/src/handlers/major_change.rs b/src/handlers/major_change.rs
@@ -6,7 +6,6 @@ use crate::{
     config::MajorChangeConfig,
     github::{Event, Issue, IssuesAction, IssuesEvent, Label, ZulipGitHubReference},
     handlers::Context,
-    interactions::ErrorComment,
 };
 use anyhow::Context as _;
 use async_trait::async_trait;
@@ -113,15 +112,10 @@ pub(super) async fn handle_input(
         .iter()
         .any(|l| l.name == config.enabling_label)
     {
-        let cmnt = ErrorComment::new(
-            &event.issue,
-            format!(
-                "This issue is not ready for proposals; it lacks the `{}` label.",
-                config.enabling_label
-            ),
-        );
-        cmnt.post(&ctx.github).await?;
-        return Ok(());
+        return user_error!(format!(
+            "This issue is not ready for proposals; it lacks the `{}` label.",
+            config.enabling_label
+        ));
     }
     let (zulip_msg, label_to_add) = match cmd {
         Invocation::NewProposal => (
@@ -253,15 +247,10 @@ pub(super) async fn handle_command(
         .iter()
         .any(|l| l.name == config.enabling_label)
     {
-        let cmnt = ErrorComment::new(
-            &issue,
-            &format!(
-                "This issue cannot be seconded; it lacks the `{}` label.",
-                config.enabling_label
-            ),
-        );
-        cmnt.post(&ctx.github).await?;
-        return Ok(());
+        return user_error!(format!(
+            "This issue cannot be seconded; it lacks the `{}` label.",
+            config.enabling_label
+        ));
     }
 
     let is_team_member = event
@@ -272,9 +261,7 @@ pub(super) async fn handle_command(
         .unwrap_or(false);
 
     if !is_team_member {
-        let cmnt = ErrorComment::new(&issue, "Only team members can second issues.");
-        cmnt.post(&ctx.github).await?;
-        return Ok(());
+        return user_error!("Only team members can second issues.");
     }
 
     let has_concerns = if let Some(concerns_label) = &config.concerns_label {
diff --git a/src/handlers/nominate.rs b/src/handlers/nominate.rs
@@ -21,15 +21,9 @@ pub(super) async fn handle_command(
     };
 
     if !is_team_member {
-        let cmnt = ErrorComment::new(
-            &event.issue().unwrap(),
-            format!(
-                "Nominating and approving issues and pull requests is restricted to members of\
-                 the Rust teams."
-            ),
+        return user_error!(
+            "Nominating and approving issues and pull requests is restricted to members of the Rust teams."
         );
-        cmnt.post(&ctx.github).await?;
-        return Ok(());
     }
 
     let issue_labels = event.issue().unwrap().labels();
diff --git a/src/handlers/ping.rs b/src/handlers/ping.rs
@@ -8,7 +8,6 @@ use crate::{
     config::PingConfig,
     github::{self, Event},
     handlers::Context,
-    interactions::ErrorComment,
 };
 use parser::command::ping::PingCommand;
 
@@ -25,42 +24,27 @@ pub(super) async fn handle_command(
     };
 
     if !is_team_member {
-        let cmnt = ErrorComment::new(
-            &event.issue().unwrap(),
-            format!("Only Rust team members can ping teams."),
-        );
-        cmnt.post(&ctx.github).await?;
-        return Ok(());
+        return user_error!("Only Rust team members can ping teams.");
     }
 
     let (gh_team, config) = match config.get_by_name(&team_name.team) {
         Some(v) => v,
         None => {
-            let cmnt = ErrorComment::new(
-                &event.issue().unwrap(),
-                format!(
-                    "This team (`{}`) cannot be pinged via this command; \
+            return user_error!(format!(
+                "This team (`{}`) cannot be pinged via this command; \
                     it may need to be added to `triagebot.toml` on the default branch.",
-                    team_name.team,
-                ),
-            );
-            cmnt.post(&ctx.github).await?;
-            return Ok(());
+                team_name.team,
+            ));
         }
     };
     let team = ctx.team.get_team(&gh_team).await?;
     let team = match team {
         Some(team) => team,
         None => {
-            let cmnt = ErrorComment::new(
-                &event.issue().unwrap(),
-                format!(
-                    "This team (`{}`) does not exist in the team repository.",
-                    team_name.team,
-                ),
-            );
-            cmnt.post(&ctx.github).await?;
-            return Ok(());
+            return user_error!(format!(
+                "This team (`{}`) does not exist in the team repository.",
+                team_name.team,
+            ));
         }
     };
 
@@ -76,11 +60,7 @@ pub(super) async fn handle_command(
             )
             .await
         {
-            let cmnt = ErrorComment::new(
-                &event.issue().unwrap(),
-                format!("Error adding team label (`{}`): {:?}.", label, err),
-            );
-            cmnt.post(&ctx.github).await?;
+            return user_error!(format!("Error adding team label (`{}`): {:?}.", label, err));
         }
     }
 
diff --git a/src/handlers/relabel.rs b/src/handlers/relabel.rs
diff --git a/src/handlers/shortcut.rs b/src/handlers/shortcut.rs

Original file line number	Diff line number	Diff line change
`@@ -274,11 +274,10 @@ async fn process_payload(`
`274`	`274`	`}`
`275`	`275`	`}`
`276`	`276`	`if !message.is_empty() {`
	`277`	`+ log::info!("user error: {}", message);`
`277`	`278`	`if let Some(issue) = event.issue() {`
`278`	`279`	`let cmnt = ErrorComment::new(issue, message);`
`279`	`280`	`cmnt.post(&ctx.github).await?;`
`280`		`- } else {`
`281`		`- log::error!("handling event failed: {:?}", message);`
`282`	`281`	`}`
`283`	`282`	`}`
`284`	`283`	`if other_error {`