From 1a554f967c7132eda3b6727ff4376008cbd8d496 Mon Sep 17 00:00:00 2001 From: Urgau Date: Tue, 17 Jun 2025 19:38:55 +0200 Subject: [PATCH 1/3] Add errors handling check-commits super-handler --- src/handlers/check_commits.rs | 62 +++++++++++++++++++++++++++++++---- 1 file changed, 56 insertions(+), 6 deletions(-) diff --git a/src/handlers/check_commits.rs b/src/handlers/check_commits.rs index eb890180..eac4de66 100644 --- a/src/handlers/check_commits.rs +++ b/src/handlers/check_commits.rs @@ -21,11 +21,14 @@ mod no_merges; mod non_default_branch; /// 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, /// ID of the most recent warning comment. @@ -61,6 +64,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 +112,44 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any } } - handle_warnings_and_labels(ctx, event, warnings, labels).await + 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, warnings: Vec, labels: Vec, ) -> 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 +253,28 @@ fn calculate_label_changes( (removals, additions) } +// Calculate the error changes +fn calculate_error_changes( + previous: &Vec<(String, String)>, + current: &Vec, +) -> (Vec<(String, String)>, Vec) { + let previous_set: HashSet<(String, String)> = previous.into_iter().cloned().collect(); + let current_set: HashSet = 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}; From 5648e1f887e6f0396e8155bb5421436865b6d8e8 Mon Sep 17 00:00:00 2001 From: Urgau Date: Tue, 17 Jun 2025 19:41:55 +0200 Subject: [PATCH 2/3] Remove old `triagebot.toml` validation handler --- src/config.rs | 14 ---- src/handlers.rs | 2 - src/handlers/validate_config.rs | 128 -------------------------------- 3 files changed, 144 deletions(-) delete mode 100644 src/handlers/validate_config.rs diff --git a/src/config.rs b/src/config.rs index b02904e8..fca6b8cf 100644 --- a/src/config.rs +++ b/src/config.rs @@ -38,9 +38,6 @@ pub(crate) struct Config { pub(crate) concern: 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, pub(crate) pr_tracking: Option, pub(crate) transfer: Option, pub(crate) merge_conflicts: Option, @@ -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 { - 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, diff --git a/src/handlers.rs b/src/handlers.rs index 594c6895..7b14a5bc 100644 --- a/src/handlers.rs +++ b/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 { 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 { diff --git a/src/handlers/validate_config.rs b/src/handlers/validate_config.rs deleted file mode 100644 index 5eb4c698..00000000 --- a/src/handlers/validate_config.rs +++ /dev/null @@ -1,128 +0,0 @@ -//! 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::{CONFIG_FILE_NAME, ValidateConfig}, - github::IssuesAction, - handlers::{Context, IssuesEvent}, -}; -use tracing as log; - -pub(super) async fn parse_input( - ctx: &Context, - event: &IssuesEvent, - _config: Option<&ValidateConfig>, -) -> Result, String> { - if !matches!( - event.action, - IssuesAction::Opened | IssuesAction::Reopened | IssuesAction::Synchronize - ) { - return Ok(None); - } - // 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.filename == 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 Some(repo) = &pr_source.repo else { - log::warn!("repo is not available in {event:?}"); - return Ok(None); - }; - let triagebot_content = match ctx - .github - .raw_file(&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}", - 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) -} From adf7cdbdbce42bae6bf4f27240ccb7712b4e9f19 Mon Sep 17 00:00:00 2001 From: Urgau Date: Tue, 17 Jun 2025 19:44:44 +0200 Subject: [PATCH 3/3] Add new simplified `triagebot.toml` validation for PR --- src/handlers/check_commits.rs | 13 ++- src/handlers/check_commits/validate_config.rs | 90 +++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 src/handlers/check_commits/validate_config.rs diff --git a/src/handlers/check_commits.rs b/src/handlers/check_commits.rs index eac4de66..678234a3 100644 --- a/src/handlers/check_commits.rs +++ b/src/handlers/check_commits.rs @@ -19,6 +19,7 @@ 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_KEY: &str = "check-commits-warnings"; @@ -44,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(()); @@ -112,6 +116,13 @@ pub(super) async fn handle(ctx: &Context, event: &Event, config: &Config) -> any } } + // 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 } diff --git a/src/handlers/check_commits/validate_config.rs b/src/handlers/check_commits/validate_config.rs new file mode 100644 index 00000000..46964b60 --- /dev/null +++ b/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> { + 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::(&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) +}