Merge pull request #2084 from Urgau/move-validate_config-to-check_commits

Move `triagebot.toml` validation handler to check commits super-handler

Author: Urgau

src/config.rs

@@ -38,9 +38,6 @@ pub(crate) struct Config {
     pub(crate) concern: Option<ConcernConfig>,
     pub(crate) mentions: Option<MentionsConfig>,
     pub(crate) no_merges: Option<NoMergesConfig>,
-    // We want this validation to run even without the entry in the config file
-    #[serde(default = "ValidateConfig::default")]
-    pub(crate) validate_config: Option<ValidateConfig>,
     pub(crate) pr_tracking: Option<ReviewPrefsConfig>,
     pub(crate) transfer: Option<TransferConfig>,
     pub(crate) merge_conflicts: Option<MergeConflictConfig>,
@@ -254,15 +251,6 @@ pub(crate) struct PrioritizeConfig {
     pub(crate) label: String,
 }
 
-#[derive(PartialEq, Eq, Debug, serde::Deserialize)]
-pub(crate) struct ValidateConfig {}
-
-impl ValidateConfig {
-    fn default() -> Option<Self> {
-        Some(ValidateConfig {})
-    }
-}
-
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
 pub(crate) struct AutolabelConfig {
     #[serde(flatten)]
@@ -722,7 +710,6 @@ mod tests {
                 review_requested: None,
                 mentions: None,
                 no_merges: None,
-                validate_config: Some(ValidateConfig {}),
                 pr_tracking: None,
                 transfer: None,
                 merge_conflicts: None,
@@ -813,7 +800,6 @@ mod tests {
                 review_requested: None,
                 mentions: None,
                 no_merges: None,
-                validate_config: Some(ValidateConfig {}),
                 pr_tracking: None,
                 transfer: None,
                 merge_conflicts: None,

src/handlers.rs

@@ -57,7 +57,6 @@ pub mod rustc_commits;
 mod shortcut;
 mod transfer;
 pub mod types_planning_updates;
-mod validate_config;
 
 pub async fn handle(ctx: &Context, event: &Event) -> Vec<HandlerError> {
     let config = config::get(&ctx.github, event.repo()).await;
@@ -232,7 +231,6 @@ issue_handlers! {
     notify_zulip,
     review_requested,
     pr_tracking,
-    validate_config,
 }
 
 macro_rules! command_handlers {

src/handlers/check_commits.rs

@@ -19,13 +19,17 @@ mod modified_submodule;
 mod no_mentions;
 mod no_merges;
 mod non_default_branch;
+mod validate_config;
 
 /// Key for the state in the database
-const CHECK_COMMITS_WARNINGS_KEY: &str = "check-commits-warnings";
+const CHECK_COMMITS_KEY: &str = "check-commits-warnings";
 
 /// State stored in the database
 #[derive(Debug, Default, serde::Deserialize, serde::Serialize, Clone, PartialEq)]
-struct CheckCommitsWarningsState {
+struct CheckCommitsState {
+    /// List of the last errors (comment body, comment node-id).
+    #[serde(default)]
+    last_errors: Vec<(String, String)>,
     /// List of the last warnings in the most recent comment.
     last_warnings: Vec<String>,
     /// ID of the most recent warning comment.
@@ -41,7 +45,10 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any
 
     if !matches!(
         event.action,
-        IssuesAction::Opened | IssuesAction::Synchronize | IssuesAction::ReadyForReview
+        IssuesAction::Opened
+            | IssuesAction::Reopened
+            | IssuesAction::Synchronize
+            | IssuesAction::ReadyForReview
     ) || !event.issue.is_pr()
     {
         return Ok(());
@@ -61,6 +68,7 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any
     let commits = event.issue.commits(&ctx.github).await?;
     let diff = &compare.files;
 
+    let mut errors = Vec::new();
     let mut warnings = Vec::new();
     let mut labels = Vec::new();
 
@@ -108,20 +116,51 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any
         }
     }
 
-    handle_warnings_and_labels(ctx, event, warnings, labels).await
+    // Check if the `triagebot.toml` config is valid
+    errors.extend(
+        validate_config::validate_config(ctx, &event, diff)
+            .await
+            .context("validating the the triagebot config")?,
+    );
+
+    handle_new_state(ctx, event, errors, warnings, labels).await
 }
 
 // Add, hide or hide&add a comment with the warnings.
-async fn handle_warnings_and_labels(
+async fn handle_new_state(
     ctx: &Context,
     event: &IssuesEvent,
+    errors: Vec<String>,
     warnings: Vec<String>,
     labels: Vec<String>,
 ) -> anyhow::Result<()> {
     // Get the state of the warnings for this PR in the database.
     let mut db = ctx.db.get().await;
-    let mut state: IssueData<'_, CheckCommitsWarningsState> =
-        IssueData::load(&mut db, &event.issue, CHECK_COMMITS_WARNINGS_KEY).await?;
+    let mut state: IssueData<'_, CheckCommitsState> =
+        IssueData::load(&mut db, &event.issue, CHECK_COMMITS_KEY).await?;
+
+    // Handles the errors, post the new ones, hide resolved ones and don't touch the one still active
+    if !state.data.last_errors.is_empty() || !errors.is_empty() {
+        let (errors_to_remove, errors_to_add) =
+            calculate_error_changes(&state.data.last_errors, &errors);
+
+        for error_to_remove in errors_to_remove {
+            event
+                .issue
+                .hide_comment(
+                    &ctx.github,
+                    &error_to_remove.1,
+                    ReportedContentClassifiers::Resolved,
+                )
+                .await?;
+            state.data.last_errors.retain(|e| e != &error_to_remove);
+        }
+
+        for error_to_add in errors_to_add {
+            let comment = event.issue.post_comment(&ctx.github, &error_to_add).await?;
+            state.data.last_errors.push((error_to_add, comment.node_id));
+        }
+    }
 
     // We only post a new comment when we haven't posted one with the same warnings before.
     if !warnings.is_empty() && state.data.last_warnings != warnings {
@@ -225,6 +264,28 @@ fn calculate_label_changes(
     (removals, additions)
 }
 
+// Calculate the error changes
+fn calculate_error_changes(
+    previous: &Vec<(String, String)>,
+    current: &Vec<String>,
+) -> (Vec<(String, String)>, Vec<String>) {
+    let previous_set: HashSet<(String, String)> = previous.into_iter().cloned().collect();
+    let current_set: HashSet<String> = current.into_iter().cloned().collect();
+
+    let removals = previous_set
+        .iter()
+        .filter(|(e, _)| !current_set.contains(e))
+        .cloned()
+        .collect();
+    let additions = current_set
+        .iter()
+        .filter(|e| !previous_set.iter().any(|(e2, _)| e == &e2))
+        .cloned()
+        .collect();
+
+    (removals, additions)
+}
+
 #[cfg(test)]
 fn dummy_commit_from_body(sha: &str, body: &str) -> GithubCommit {
     use chrono::{DateTime, FixedOffset};

src/handlers/check_commits/validate_config.rs

@@ -0,0 +1,90 @@
+//! For pull requests that have changed the triagebot.toml, validate that the
+//! changes are a valid configuration file.
+
+use crate::{
+    config::CONFIG_FILE_NAME,
+    github::FileDiff,
+    handlers::{Context, IssuesEvent},
+};
+use anyhow::{Context as _, bail};
+
+pub(super) async fn validate_config(
+    ctx: &Context,
+    event: &IssuesEvent,
+    diff: &[FileDiff],
+) -> anyhow::Result<Option<String>> {
+    if !diff.iter().any(|diff| diff.filename == CONFIG_FILE_NAME) {
+        return Ok(None);
+    }
+
+    let Some(pr_source) = &event.issue.head else {
+        bail!("expected head commit");
+    };
+    let Some(repo) = &pr_source.repo else {
+        bail!("repo is not available");
+    };
+
+    let triagebot_content = ctx
+        .github
+        .raw_file(&repo.full_name, &pr_source.sha, CONFIG_FILE_NAME)
+        .await
+        .context("{CONFIG_FILE_NAME} modified, but failed to get content")?;
+
+    let triagebot_content = triagebot_content.unwrap_or_default();
+    let triagebot_content = String::from_utf8_lossy(&*triagebot_content);
+
+    let Err(e) = toml::from_str::<crate::handlers::Config>(&triagebot_content) else {
+        return Ok(None);
+    };
+
+    let position = match e.span() {
+        // toml sometimes gives bad spans, see https://github.com/toml-rs/toml/issues/589
+        Some(span) if span != (0..0) => {
+            let (line, col) = translate_position(&triagebot_content, span.start);
+            let url = format!(
+                "https://github.com/{}/blob/{}/{CONFIG_FILE_NAME}#L{line}",
+                repo.full_name, pr_source.sha
+            );
+            format!(" at position [{line}:{col}]({url})",)
+        }
+        Some(_) | None => String::new(),
+    };
+
+    Ok(Some(format!(
+        "Invalid `triagebot.toml`{position}:\n\
+        `````\n\
+        {e}\n\
+        `````",
+    )))
+}
+
+/// Helper to translate a toml span to a `(line_no, col_no)` (1-based).
+fn translate_position(input: &str, index: usize) -> (usize, usize) {
+    if input.is_empty() {
+        return (0, index);
+    }
+
+    let safe_index = index.min(input.len() - 1);
+    let column_offset = index - safe_index;
+
+    let nl = input[0..safe_index]
+        .as_bytes()
+        .iter()
+        .rev()
+        .enumerate()
+        .find(|(_, b)| **b == b'\n')
+        .map(|(nl, _)| safe_index - nl - 1);
+    let line_start = match nl {
+        Some(nl) => nl + 1,
+        None => 0,
+    };
+    let line = input[0..line_start]
+        .as_bytes()
+        .iter()
+        .filter(|c| **c == b'\n')
+        .count();
+    let column = input[line_start..=safe_index].chars().count() - 1;
+    let column = column + column_offset;
+
+    (line + 1, column + 1)
+}