diff --git a/Cargo.lock b/Cargo.lock index 72099b3d..73f1cc3c 100644 --- a/Cargo.lock +++ b/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" diff --git a/Cargo.toml b/Cargo.toml index eaf1ed15..27736094 100644 --- a/Cargo.toml +++ b/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"] } diff --git a/src/config.rs b/src/config.rs index 8312aab3..244c35ef 100644 --- a/src/config.rs +++ b/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, pub(crate) assign: Option, @@ -35,9 +36,13 @@ pub(crate) struct Config { pub(crate) note: Option, pub(crate) mentions: Option, pub(crate) no_merges: Option, + // We want this validation to run even without the entry in the config file + #[serde(default = "ValidateConfig::default")] + pub(crate) validate_config: Option, } #[derive(PartialEq, Eq, Debug, serde::Deserialize)] +#[serde(deny_unknown_fields)] pub(crate) struct NominateConfig { // team name -> label pub(crate) teams: HashMap, @@ -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, #[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, } #[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 { + 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, @@ -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, pub(crate) reviewed_label: String, } #[derive(PartialEq, Eq, Debug, serde::Deserialize)] +#[serde(deny_unknown_fields)] pub(crate) struct ReviewRequestedConfig { pub(crate) remove_labels: Vec, pub(crate) add_labels: Vec, @@ -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::(&contents).map_err(ConfigurationError::Toml)?); + let contents = String::from_utf8_lossy(&*contents); + let config = Arc::new(toml::from_str::(&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 {}), } ); } diff --git a/src/github.rs b/src/github.rs index bbd824dd..040da27d 100644 --- a/src/github.rs +++ b/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, diff --git a/src/handlers.rs b/src/handlers.rs index d2bf47d8..4838760e 100644 --- a/src/handlers.rs +++ b/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 { 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 { diff --git a/src/handlers/assign/tests/tests_candidates.rs b/src/handlers/assign/tests/tests_candidates.rs index f78aeca4..861ca7aa 100644 --- a/src/handlers/assign/tests/tests_candidates.rs +++ b/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, - config: toml::Value, + teams: Option, + 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, - config: toml::Value, + teams: Option, + config: toml::Table, issue: serde_json::Value, ) -> (Teams, AssignConfig, Issue) { // Convert the simplified team config to a real team config. diff --git a/src/handlers/assign/tests/tests_from_diff.rs b/src/handlers/assign/tests/tests_from_diff.rs index 05729cf0..7cd418be 100644 --- a/src/handlers/assign/tests/tests_from_diff.rs +++ b/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!( diff --git a/src/handlers/validate_config.rs b/src/handlers/validate_config.rs new file mode 100644 index 00000000..04814918 --- /dev/null +++ b/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, 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::(&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) +}