feat: valdiate proposed change in triagetoml for PRs ✨

fixes rust-lang/rust#106104

Author: meysam81

src/config.rs

@@ -6,7 +6,7 @@ use std::sync::{Arc, RwLock};
 use std::time::{Duration, Instant};
 use tracing as log;
 
-static CONFIG_FILE_NAME: &str = "triagebot.toml";
+pub(crate) static CONFIG_FILE_NAME: &str = "triagebot.toml";
 const REFRESH_EVERY: Duration = Duration::from_secs(2 * 60); // Every two minutes
 
 lazy_static::lazy_static! {
@@ -36,6 +36,9 @@ pub(crate) struct Config {
     pub(crate) note: Option<NoteConfig>,
     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>,
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
@@ -167,6 +170,15 @@ 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)]
@@ -450,6 +462,7 @@ mod tests {
                 review_requested: None,
                 mentions: None,
                 no_merges: None,
+                validate_config: Some(ValidateConfig {}),
             }
         );
     }

src/github.rs

@@ -999,7 +999,7 @@ struct PullRequestEventFields {}
 
 #[derive(Clone, Debug, serde::Deserialize)]
 pub struct CommitBase {
-    sha: String,
+    pub sha: String,
     #[serde(rename = "ref")]
     pub git_ref: String,
     pub repo: Repository,

src/handlers.rs

@@ -46,6 +46,7 @@ mod rfc_helper;
 pub mod rustc_commits;
 mod shortcut;
 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;
@@ -166,6 +167,7 @@ issue_handlers! {
     no_merges,
     notify_zulip,
     review_requested,
+    validate_config,
 }
 
 macro_rules! command_handlers {

src/handlers/validate_config.rs

@@ -0,0 +1,117 @@
+//! For pull requests that have changed the triagebot.toml, validate that the
+//! changes are a valid configuration file.
+//! It won't validate anything unless the PR is open and has changed.
+
+use crate::{
+    config::{ValidateConfig, CONFIG_FILE_NAME},
+    handlers::{Context, IssuesEvent},
+};
+use tracing as log;
+
+pub(super) async fn parse_input(
+    ctx: &Context,
+    event: &IssuesEvent,
+    _config: Option<&ValidateConfig>,
+) -> Result<Option<()>, String> {
+    // All processing needs to be done in parse_input (instead of
+    // handle_input) because we want this to *always* run. handle_input
+    // requires the config to exist in triagebot.toml, but we want this to run
+    // even if it isn't configured. As a consequence, error handling needs to
+    // be a little more cautious here, since we don't want to relay
+    // un-actionable errors to the user.
+    let diff = match event.issue.diff(&ctx.github).await {
+        Ok(Some(diff)) => diff,
+        Ok(None) => return Ok(None),
+        Err(e) => {
+            log::error!("failed to get diff {e}");
+            return Ok(None);
+        }
+    };
+    if !diff.iter().any(|diff| diff.path == CONFIG_FILE_NAME) {
+        return Ok(None);
+    }
+
+    let Some(pr_source) = &event.issue.head else {
+        log::error!("expected head commit in {event:?}");
+        return Ok(None);
+    };
+    let triagebot_content = match ctx
+        .github
+        .raw_file(&pr_source.repo.full_name, &pr_source.sha, CONFIG_FILE_NAME)
+        .await
+    {
+        Ok(Some(c)) => c,
+        Ok(None) => {
+            log::error!("{CONFIG_FILE_NAME} modified, but failed to get content");
+            return Ok(None);
+        }
+        Err(e) => {
+            log::error!("failed to get {CONFIG_FILE_NAME}: {e}");
+            return Ok(None);
+        }
+    };
+
+    let triagebot_content = String::from_utf8_lossy(&*triagebot_content);
+    if let Err(e) = toml::from_str::<crate::handlers::Config>(&triagebot_content) {
+        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}",
+                    pr_source.repo.full_name, pr_source.sha
+                );
+                format!(" at position [{line}:{col}]({url})",)
+            }
+            Some(_) | None => String::new(),
+        };
+
+        return Err(format!(
+            "Invalid `triagebot.toml`{position}:\n\
+            `````\n\
+            {e}\n\
+            `````",
+        ));
+    }
+    Ok(None)
+}
+
+pub(super) async fn handle_input(
+    _ctx: &Context,
+    _config: &ValidateConfig,
+    _event: &IssuesEvent,
+    _input: (),
+) -> anyhow::Result<()> {
+    Ok(())
+}
+
+/// 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)
+}