diff --git a/README.md b/README.md index 8cec324..fdb7547 100644 --- a/README.md +++ b/README.md @@ -146,12 +146,14 @@ match = { all_of = [{ event_type = "pull_request" }, { not = { draft = true } }] action = { type = "http_post", url = "https://hooks.example.test/reviews" } ``` -`all_of` and `any_of` take arrays of condition tables and reject empty -arrays; `not` takes a single condition table. A table mixing operators with -other keys — or using two operators as siblings — is rejected at config load -with an actionable error. A scalar under an operator-named key (e.g. -`not = "draft"`) is an ordinary metadata lookup, exactly as before: the -operator only applies to table-shaped values, so existing configurations +`all_of` and `any_of` take nonempty arrays of condition tables; `not` takes +one condition table. A table mixing operators with other keys — or using two +operators as siblings — is rejected at config load with an actionable error. +Structured values must use their declared grammar: an inline table under +`all_of`/`any_of`, an array under `not`, or any table/array under an ordinary +leaf key is rejected rather than silently becoming a non-matching leaf. A +scalar under an operator-named key (for example, `not = "draft"`) remains an +ordinary metadata lookup exactly as before, so existing scalar configurations never change meaning. ## Development diff --git a/crates/rite-core/src/lib.rs b/crates/rite-core/src/lib.rs index 1807dbb..f68e27a 100644 --- a/crates/rite-core/src/lib.rs +++ b/crates/rite-core/src/lib.rs @@ -12,6 +12,10 @@ use url::Url; /// Result type used by Rite domain operations. pub type Result = std::result::Result; +/// Stable marker for matcher errors that may safely cross a configuration-load +/// boundary without rendering the originating TOML source text. +pub const MATCH_VALIDATION_ERROR_PREFIX: &str = "rite matcher validation: "; + /// Errors returned by Rite components. #[derive(Debug, Error)] pub enum RiteError { @@ -116,22 +120,25 @@ pub struct RiteHandler { impl RiteHandler { /// Convert the flat TOML matcher map into a match condition tree. /// - /// Operator keys (`all_of`, `any_of`, `not`) are recognized only when - /// their TOML value is an inline table (compound form) or, for `not`, - /// exactly that; a scalar value under an operator-named key keeps its - /// legacy meaning as an ordinary metadata-lookup leaf, so existing - /// configurations never change semantics. + /// Operator keys are recognized only when their TOML values have the + /// exact compound grammar: `all_of` and `any_of` take arrays of + /// condition tables, while `not` takes one condition table. A scalar + /// under an operator-named key keeps its legacy meaning as an ordinary + /// metadata-lookup leaf, so existing configurations never change + /// semantics. /// /// # Errors /// - /// Returns a [`RiteError::Config`] when the table mixes operators with - /// other keys, or when `all_of`/`any_of` is empty, `not` is not a - /// single-child table, or any nested operand repeats a violation. + /// Returns a [`RiteError::Config`] when a structured value has the wrong + /// key or operand shape, the table mixes operators with other keys, + /// `all_of`/`any_of` is empty, `not` is not a single-child table, or any + /// nested operand repeats a violation. pub fn condition_tree(&self) -> Result { Self::tree_from_map(&self.matcher) } fn tree_from_map(map: &BTreeMap) -> Result { + Self::validate_value_shapes(map)?; // Operator keys are only operators when their TOML shape matches the // grammar: all_of/any_of take an array of condition tables, not takes // a single condition table. A scalar under an operator-named key is a @@ -166,6 +173,38 @@ impl RiteHandler { } } + fn validate_value_shapes(map: &BTreeMap) -> Result<()> { + for (key, value) in map { + match (key.as_str(), value) { + ("all_of" | "any_of", MatchValue::Array(_)) + | ("not", MatchValue::Table(_)) + | (_, MatchValue::String(_) | MatchValue::Integer(_) | MatchValue::Boolean(_)) => {} + ("all_of" | "any_of", MatchValue::Table(_)) => { + return Err(RiteError::Config(format!( + "match operator '{key}' requires an array of condition tables, not an inline table" + ))); + } + ("not", MatchValue::Array(_)) => { + return Err(RiteError::Config( + "match operator 'not' requires an inline table holding exactly one child condition, not an array" + .into(), + )); + } + (_, MatchValue::Table(_)) => { + return Err(RiteError::Config(format!( + "match key '{key}' requires a scalar string, integer, or boolean; inline tables are only valid as the operand of 'not'" + ))); + } + (_, MatchValue::Array(_)) => { + return Err(RiteError::Config(format!( + "match key '{key}' requires a scalar string, integer, or boolean; arrays are only valid as operands of 'all_of' or 'any_of'" + ))); + } + } + } + Ok(()) + } + fn children_of( map: &BTreeMap, op: &str, @@ -236,14 +275,17 @@ where let map = BTreeMap::::deserialize(deserializer)?; RiteHandler::tree_from_map(&map) .map(|_| map) - .map_err(serde::de::Error::custom) + .map_err(|error| { + serde::de::Error::custom(format!("{MATCH_VALIDATION_ERROR_PREFIX}{error}")) + }) } /// A scalar value used to match an event field or metadata item. /// -/// The `Table` and `Array` variants only appear inside compound -/// (`all_of`/`any_of`/`not`) match tables; legacy flat matchers keep their -/// scalar shapes verbatim. +/// The `Table` and `Array` variants only appear in their declared compound +/// positions: `Table` for `not`, `Array` for `all_of`/`any_of`. Legacy flat +/// matchers keep scalar shapes verbatim, and unsupported structured values +/// are rejected at deserialization time. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(untagged)] pub enum MatchValue { @@ -542,6 +584,10 @@ action = { type = "http_post", url = "https://example.test/hook" }"#, const ACTION: &str = "action = { type = \"http_post\", url = \"https://example.test/hook\" }"; + fn handler_with_match(matcher: &str) -> String { + format!("name = \"match-shape\"\nsource = \"iris\"\n{matcher}\n{ACTION}") + } + #[test] fn compound_any_of_matches_exactly_critical_or_urgent() { let handler: RiteHandler = toml::from_str(&format!( @@ -636,7 +682,7 @@ action = { type = "http_post", url = "https://example.test/hook" }"#, // A scalar under an operator-named key is a metadata lookup, not an // operator — legacy configs using those exact key names keep working. let handler: RiteHandler = toml::from_str(&format!( - "name = \"legacy-named\"\nsource = \"iris\"\nmatch = {{ not = \"a-value\", all_of = \"another\", event_type = \"text\" }}\n{ACTION}" + "name = \"legacy-named\"\nsource = \"iris\"\nmatch = {{ not = \"a-value\", all_of = \"another\", any_of = true, event_type = \"text\" }}\n{ACTION}" )) .expect("legacy scalar handler parses"); @@ -644,7 +690,7 @@ action = { type = "http_post", url = "https://example.test/hook" }"#, let MatchCondition::All(children) = tree else { panic!("scalar operator-named keys must parse to All"); }; - assert_eq!(children.len(), 3); + assert_eq!(children.len(), 4); let matching = event( "iris", @@ -654,11 +700,34 @@ action = { type = "http_post", url = "https://example.test/hook" }"#, BTreeMap::from([ ("not".into(), Value::String("a-value".into())), ("all_of".into(), Value::String("another".into())), + ("any_of".into(), Value::Bool(true)), ]), ); assert!(handler.matches(&matching)); } + #[test] + fn omitted_and_empty_legacy_matchers_remain_valid() { + let omitted = toml::from_str::(&format!( + "name = \"omitted\"\nsource = \"iris\"\n{ACTION}" + )) + .expect("omitted matcher parses"); + let empty = toml::from_str::(&handler_with_match("match = {}")) + .expect("empty matcher parses"); + let event = event("iris", "text", Severity::Info, None, BTreeMap::new()); + + assert!(matches!( + omitted.condition_tree().expect("omitted tree"), + MatchCondition::All(children) if children.is_empty() + )); + assert!(matches!( + empty.condition_tree().expect("empty tree"), + MatchCondition::All(children) if children.is_empty() + )); + assert!(omitted.matches(&event)); + assert!(empty.matches(&event)); + } + #[test] fn compound_rejects_empty_children_and_misshapen_not() { // Malformed compound tables are rejected at deserialization time — @@ -703,6 +772,82 @@ action = { type = "http_post", url = "https://example.test/hook" }"#, assert!(siblings.to_string().contains("multiple operators")); } + #[test] + fn compound_rejects_wrong_structured_operand_shapes_at_load() { + for (label, matcher, key) in [ + ( + "all_of table", + "match = { all_of = { event_type = \"push\" } }", + "all_of", + ), + ( + "any_of table", + "match = { any_of = { event_type = \"push\" } }", + "any_of", + ), + ( + "not array", + "match = { not = [{ event_type = \"push\" }] }", + "not", + ), + ] { + let error = toml::from_str::(&handler_with_match(matcher)) + .expect_err(&format!("{label} must be rejected at load")); + assert!( + error.to_string().contains(key), + "{label} diagnostic must name its match key: {error}" + ); + } + } + + #[test] + fn compound_rejects_structured_leaves_and_nested_wrong_shapes_at_load() { + for (label, matcher, key) in [ + ( + "ordinary table leaf", + "match = { metadata = { channel = \"alerts\" } }", + "metadata", + ), + ( + "ordinary array leaf", + "match = { labels = [{ channel = \"alerts\" }] }", + "labels", + ), + ( + "ordinary table leaf under not", + "match = { not = { metadata = { channel = \"alerts\" } } }", + "metadata", + ), + ( + "ordinary array leaf under any_of", + "match = { any_of = [{ labels = [{ channel = \"alerts\" }] }] }", + "labels", + ), + ( + "wrong all_of shape under not", + "match = { not = { all_of = { event_type = \"push\" } } }", + "all_of", + ), + ( + "wrong any_of shape under all_of", + "match = { all_of = [{ any_of = { event_type = \"push\" } }] }", + "any_of", + ), + ( + "wrong not shape under any_of", + "match = { any_of = [{ not = [{ event_type = \"push\" }] }] }", + "not", + ), + ] { + let error = toml::from_str::(&handler_with_match(matcher)) + .expect_err(&format!("{label} must be rejected at load")); + assert!( + error.to_string().contains(key), + "{label} diagnostic must name its match key: {error}" + ); + } + } + #[test] fn programmatic_matcher_corruption_is_still_rejected() { // Deserialization covers every TOML path; condition_tree() still diff --git a/crates/rite-server/src/lib.rs b/crates/rite-server/src/lib.rs index f4a5705..379fb44 100644 --- a/crates/rite-server/src/lib.rs +++ b/crates/rite-server/src/lib.rs @@ -12,7 +12,7 @@ use std::{ }; use axum::{Router, routing::get}; -use rite_core::{EventSource, RiteAction, RiteEvent, RiteHandler}; +use rite_core::{EventSource, MATCH_VALIDATION_ERROR_PREFIX, RiteAction, RiteEvent, RiteHandler}; use rite_sources::{github::GitHubSource, iris::IrisSource, uptime_kuma::UptimeKumaSource}; use serde::Deserialize; @@ -117,9 +117,39 @@ pub fn app(state: AppState) -> Router { .with_state(state) } -/// Parse handler configuration from TOML text. -pub fn load_config(input: &str) -> Result { - toml::from_str(input) +/// A configuration-load failure safe to display in CLI output and logs. +/// +/// Parser errors can retain a copy of their complete source document for span +/// rendering. This type deliberately retains only a known-safe matcher +/// diagnostic or a generic parse/type error, never raw TOML values. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ConfigLoadError(String); + +impl ConfigLoadError { + fn from_toml(error: &toml::de::Error) -> Self { + let message = error.message(); + let safe_message = message + .strip_prefix(MATCH_VALIDATION_ERROR_PREFIX) + .map_or_else( + || "configuration syntax or value type is invalid".to_owned(), + ToOwned::to_owned, + ); + Self(safe_message) + } +} + +impl std::fmt::Display for ConfigLoadError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(formatter, "invalid Rite configuration: {}", self.0) + } +} + +impl std::error::Error for ConfigLoadError {} + +/// Parse handler configuration from TOML text without retaining raw input in +/// user-facing errors. +pub fn load_config(input: &str) -> Result { + toml::from_str(input).map_err(|error| ConfigLoadError::from_toml(&error)) } /// Validate configuration without performing network or filesystem I/O. @@ -892,36 +922,103 @@ mod tests { fn example_and_readme_configs_load_and_validate() { let readme = include_str!("../../../README.md"); let example = include_str!("../../../rite.example.toml"); - for (label, text) in [("README.md", readme), ("rite.example.toml", example)] { - for fence in text.split("```toml").skip(1) { - let toml_text = fence.split("```").next().unwrap_or_default(); - if !toml_text.contains("[[rites]]") { - continue; - } - let config = load_config(toml_text) - .unwrap_or_else(|e| panic!("{label} fence failed to parse: {e}\n{toml_text}")); - for handler in &config.rites { - handler.condition_tree().unwrap_or_else(|e| { - panic!("{label} fence has invalid match: {e}\n{toml_text}") - }); - } - // Config fences are intentionally partial (no sources), so - // unknown-source errors are expected; match-table errors are - // not. - let config_errors = validate(&config) - .into_iter() - .filter(|d| { - d.level == DiagnosticLevel::Error && !d.message.contains("unknown source") - }) - .count(); - assert_eq!( - config_errors, 0, - "{label} fence produced validation errors:\n{toml_text}" - ); + let example_config = load_config(example) + .unwrap_or_else(|e| panic!("rite.example.toml failed to parse: {e}")); + let example_names = example_config + .rites + .iter() + .map(|handler| handler.name.as_str()) + .collect::>(); + assert_eq!(example_names, ["github-pr-opened", "discord-urgent-alert"]); + assert!( + validate(&example_config) + .iter() + .all(|diagnostic| diagnostic.level != DiagnosticLevel::Error), + "rite.example.toml must validate without errors" + ); + let malformed_example = example.replacen( + "match = { event_type = \"pull_request\", action = \"opened\" }", + "match = { all_of = { event_type = \"pull_request\" } }", + 1, + ); + assert!( + load_config(&malformed_example).is_err(), + "fixture validation must exercise the real loader" + ); + + let readme_configs = readme + .split("```toml") + .skip(1) + .filter_map(|fence| fence.split("```").next()) + .filter(|toml_text| toml_text.contains("[[rites]]")) + .collect::>(); + assert_eq!( + readme_configs.len(), + 3, + "expected three handler TOML examples" + ); + for toml_text in readme_configs { + let config = load_config(toml_text) + .unwrap_or_else(|e| panic!("README.md fence failed to parse: {e}\n{toml_text}")); + assert!( + !config.rites.is_empty(), + "README handler fence must be nonempty" + ); + for handler in &config.rites { + handler.condition_tree().unwrap_or_else(|e| { + panic!("README.md fence has invalid match: {e}\n{toml_text}") + }); } + // README fences deliberately omit source definitions when the + // example is about a handler alone. No other validation error is + // acceptable. + let unexpected_errors = validate(&config) + .into_iter() + .filter(|diagnostic| { + diagnostic.level == DiagnosticLevel::Error + && !diagnostic.message.contains("unknown source") + }) + .count(); + assert_eq!( + unexpected_errors, 0, + "README.md fence produced validation errors:\n{toml_text}" + ); } } + #[test] + fn load_config_redacts_source_values_but_keeps_match_diagnostics() { + let malformed_match = r#" +[[rites]] +name = "unsafe-match" +source = "iris" +match = { labels = [{ category = "alerts" }] } +action = { type = "http_post", url = "https://example.test/hook", headers = { authorization = "synthetic-config-marker" } } +"#; + let match_error = load_config(malformed_match).expect_err("bad match must not load"); + let match_message = match_error.to_string(); + assert!( + match_message.contains("labels"), + "safe matcher diagnostic must name the invalid key: {match_message}" + ); + assert!( + !match_message.contains("synthetic-config-marker"), + "matcher load errors must never render raw config values" + ); + + let malformed_type = r#" +[[rites]] +name = "unsafe-type" +source = { token = "synthetic-config-marker" } +action = { type = "http_post", url = "https://example.test/hook" } +"#; + let type_error = load_config(malformed_type).expect_err("bad source type must not load"); + assert!( + !type_error.to_string().contains("synthetic-config-marker"), + "generic parser errors must never render raw config values" + ); + } + #[test] fn validate_rejects_malformed_compound_match_tables() { // Malformed compound tables are rejected at load_config itself @@ -931,6 +1028,27 @@ mod tests { for (label, matcher) in [ ("empty all_of", "match = { all_of = [] }"), ("empty any_of", "match = { any_of = [] }"), + ( + "all_of table", + "match = { all_of = { event_type = \"chat\" } }", + ), + ( + "any_of table", + "match = { any_of = { event_type = \"chat\" } }", + ), + ("not array", "match = { not = [{ event_type = \"chat\" }] }"), + ( + "ordinary structured leaf", + "match = { metadata = { event_type = \"chat\" } }", + ), + ( + "nested ordinary structured leaf", + "match = { any_of = [{ labels = [{ event_type = \"chat\" }] }] }", + ), + ( + "nested malformed negation", + "match = { not = { all_of = { event_type = \"chat\" } } }", + ), ( "multi-child not", "match = { not = { severity = \"info\", event_type = \"chat\" } }",