Merge pull request #1730 from meysam81/master

feat: validate proposed change(s) in triagebot.toml for PRs ✨

Cargo.lock

@@ -508,6 +508,12 @@ version = "1.0.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "b5320ae4c3782150d900b79807611a59a99fc9a1d61d686faafc24b93fc8d7ca"
 
+[[package]]
+name = "equivalent"
+version = "1.0.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5443807d6dff69373d433ab9ef5378ad8df50ca6298caf15de6e52e24aaf54d5"
+
 [[package]]
 name = "fake-simd"
 version = "0.1.2"
@@ -760,7 +766,7 @@ dependencies = [
  "futures-sink",
  "futures-util",
  "http",
- "indexmap",
+ "indexmap 1.8.1",
  "slab",
  "tokio",
  "tokio-util 0.7.1",
@@ -782,6 +788,12 @@ dependencies = [
  "ahash",
 ]
 
+[[package]]
+name = "hashbrown"
+version = "0.14.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "290f1a1d9242c78d09ce40a5e87e7554ee637af1351968159f4952f028f75604"
+
 [[package]]
 name = "heck"
 version = "0.4.1"
@@ -962,6 +974,16 @@ dependencies = [
  "serde",
 ]
 
+[[package]]
+name = "indexmap"
+version = "2.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d530e1a18b1cb4c484e6e34556a0d948706958449fca0cab753d649f2bce3d1f"
+dependencies = [
+ "equivalent",
+ "hashbrown 0.14.3",
+]
+
 [[package]]
 name = "instant"
 version = "0.1.12"
@@ -1069,9 +1091,9 @@ dependencies = [
 
 [[package]]
 name = "memchr"
-version = "2.4.1"
+version = "2.7.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "308cc39be01b73d0d18f82a0e7b2a3df85245f84af96fdddc5d202d27e47b86a"
+checksum = "523dc4f511e55ab87b694dc30d0f820d60906ef06413f93d4d7a1385599cc149"
 
 [[package]]
 name = "mime"
@@ -1806,7 +1828,7 @@ name = "rust_team_data"
 version = "1.0.0"
 source = "git+https://github.com/rust-lang/team#49683ec8af9ca6b546b3af516ea4654424e302eb"
 dependencies = [
- "indexmap",
+ "indexmap 1.8.1",
  "serde",
 ]
 
@@ -1978,6 +2000,15 @@ dependencies = [
  "serde",
 ]
 
+[[package]]
+name = "serde_spanned"
+version = "0.6.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "eb3622f419d1296904700073ea6cc23ad690adbd66f13ea683df73298736f0c1"
+dependencies = [
+ "serde",
+]
+
 [[package]]
 name = "serde_urlencoded"
 version = "0.7.1"
@@ -2383,11 +2414,36 @@ dependencies = [
 
 [[package]]
 name = "toml"
-version = "0.5.8"
+version = "0.8.8"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a1a195ec8c9da26928f773888e0742ca3ca1040c6cd859c919c9f59c1954ab35"
+dependencies = [
+ "serde",
+ "serde_spanned",
+ "toml_datetime",
+ "toml_edit",
+]
+
+[[package]]
+name = "toml_datetime"
+version = "0.6.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3550f4e9685620ac18a50ed434eb3aec30db8ba93b0287467bca5826ea25baf1"
+dependencies = [
+ "serde",
+]
+
+[[package]]
+name = "toml_edit"
+version = "0.21.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a31142970826733df8241ef35dc040ef98c679ab14d7c3e54d827099b3acecaa"
+checksum = "d34d383cd00a163b4a5b85053df514d45bc330f6de7737edfe0a93311d1eaa03"
 dependencies = [
+ "indexmap 2.1.0",
  "serde",
+ "serde_spanned",
+ "toml_datetime",
+ "winnow",
 ]
 
 [[package]]
@@ -2958,6 +3014,15 @@ version = "0.48.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "ed94fce61571a4006852b7389a063ab983c02eb1bb37b47f8272ce92d06d9538"
 
+[[package]]
+name = "winnow"
+version = "0.5.34"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "b7cf47b659b318dccbd69cc4797a39ae128f533dce7902a1096044d1967b9c16"
+dependencies = [
+ "memchr",
+]
+
 [[package]]
 name = "winreg"
 version = "0.50.0"

Cargo.toml

@@ -18,7 +18,7 @@ hex = "0.4"
 parser = { path = "parser" }
 rust_team_data = { git = "https://github.com/rust-lang/team" }
 glob = "0.3.0"
-toml = "0.5.1"
+toml = "0.8.8"
 hyper = { version = "0.14.4", features = ["server", "stream"]}
 tokio = { version = "1.7.1", features = ["macros", "time", "rt"] }
 futures = { version = "0.3", default-features = false, features = ["std"] }

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! {
@@ -17,6 +17,7 @@ lazy_static::lazy_static! {
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
 #[serde(rename_all = "kebab-case")]
+#[serde(deny_unknown_fields)]
 pub(crate) struct Config {
     pub(crate) relabel: Option<RelabelConfig>,
     pub(crate) assign: Option<AssignConfig>,
@@ -35,9 +36,13 @@ 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)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct NominateConfig {
     // team name -> label
     pub(crate) teams: HashMap<String, String>,
@@ -68,6 +73,7 @@ impl PingConfig {
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct PingTeamConfig {
     pub(crate) message: String,
     #[serde(default)]
@@ -76,6 +82,7 @@ pub(crate) struct PingTeamConfig {
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct AssignConfig {
     /// If `true`, then posts a warning comment if the PR is opened against a
     /// different branch than the default (usually master or main).
@@ -105,6 +112,7 @@ impl AssignConfig {
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct NoMergesConfig {
     /// No action will be taken on PRs with these substrings in the title.
     #[serde(default)]
@@ -121,6 +129,7 @@ pub(crate) struct NoMergesConfig {
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct NoteConfig {
     #[serde(default)]
     _empty: (),
@@ -133,6 +142,7 @@ pub(crate) struct MentionsConfig {
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct MentionsPathConfig {
     pub(crate) message: Option<String>,
     #[serde(default)]
@@ -141,22 +151,34 @@ pub(crate) struct MentionsPathConfig {
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
 #[serde(rename_all = "kebab-case")]
+#[serde(deny_unknown_fields)]
 pub(crate) struct RelabelConfig {
     #[serde(default)]
     pub(crate) allow_unauthenticated: Vec<String>,
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct ShortcutConfig {
     #[serde(default)]
     _empty: (),
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 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)]
@@ -176,6 +198,7 @@ impl AutolabelConfig {
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct AutolabelLabelConfig {
     #[serde(default)]
     pub(crate) trigger_labels: Vec<String>,
@@ -196,6 +219,7 @@ pub(crate) struct NotifyZulipConfig {
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct NotifyZulipLabelConfig {
     pub(crate) zulip_stream: u64,
     pub(crate) topic: String,
@@ -208,6 +232,7 @@ pub(crate) struct NotifyZulipLabelConfig {
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct MajorChangeConfig {
     /// A username (typically a group, e.g. T-lang) to ping on Zulip for newly
     /// opened proposals.
@@ -243,18 +268,22 @@ impl MajorChangeConfig {
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct GlacierConfig {}
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct CloseConfig {}
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct ReviewSubmittedConfig {
     pub(crate) review_labels: Vec<String>,
     pub(crate) reviewed_label: String,
 }
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
+#[serde(deny_unknown_fields)]
 pub(crate) struct ReviewRequestedConfig {
     pub(crate) remove_labels: Vec<String>,
     pub(crate) add_labels: Vec<String>,
@@ -280,6 +309,7 @@ pub(crate) async fn get(
 
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
 #[serde(rename_all = "kebab-case")]
+#[serde(deny_unknown_fields)]
 pub(crate) struct GitHubReleasesConfig {
     pub(crate) format: ChangelogFormat,
     pub(crate) project_name: String,
@@ -307,7 +337,8 @@ async fn get_fresh_config(
         .await
         .map_err(|e| ConfigurationError::Http(Arc::new(e)))?
         .ok_or(ConfigurationError::Missing)?;
-    let config = Arc::new(toml::from_slice::<Config>(&contents).map_err(ConfigurationError::Toml)?);
+    let contents = String::from_utf8_lossy(&*contents);
+    let config = Arc::new(toml::from_str::<Config>(&contents).map_err(ConfigurationError::Toml)?);
     log::debug!("fresh configuration for {}: {:?}", repo.full_name, config);
     Ok(config)
 }
@@ -431,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/assign/tests/tests_candidates.rs

@@ -4,8 +4,8 @@ use super::super::*;
 
 /// Basic test function for testing `candidate_reviewers_from_names`.
 fn test_from_names(
-    teams: Option<toml::Value>,
-    config: toml::Value,
+    teams: Option<toml::Table>,
+    config: toml::Table,
     issue: serde_json::Value,
     names: &[&str],
     expected: Result<&[&str], FindReviewerError>,
@@ -32,8 +32,8 @@ fn test_from_names(
 
 /// Convert the simplified input in preparation for `candidate_reviewers_from_names`.
 fn convert_simplified(
-    teams: Option<toml::Value>,
-    config: toml::Value,
+    teams: Option<toml::Table>,
+    config: toml::Table,
     issue: serde_json::Value,
 ) -> (Teams, AssignConfig, Issue) {
     // Convert the simplified team config to a real team config.

src/handlers/assign/tests/tests_from_diff.rs

@@ -5,7 +5,7 @@ use crate::config::AssignConfig;
 use crate::github::parse_diff;
 use std::fmt::Write;
 
-fn test_from_diff(diff: &str, config: toml::Value, expected: &[&str]) {
+fn test_from_diff(diff: &str, config: toml::Table, expected: &[&str]) {
     let files = parse_diff(diff);
     let aconfig: AssignConfig = config.try_into().unwrap();
     assert_eq!(

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)
+}