diff --git a/CHANGELOG.md b/CHANGELOG.md index 8722541..7c0e9e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — Config Validation & Best Practices (#334) + +- **Quality gate no longer fails when disabled (#58).** `evaluate()` forced `GateResult::Pass` when `enabled: false`; it now short-circuits before applying thresholds so a disabled gate never blocks a merge. +- **Category actions are now validated enums (#57).** `quality_gate.categories.*.action` accepted any string, so a typo like `blok` silently became blocking. It is now a case-insensitive `CategoryAction` enum (`block`/`warn`/`ignore`); unknown values fail loudly at config load. +- **Config values are validated (#94).** `Config::validate()` now rejects out-of-range/unsupported values at load time: `temperature` (0.0–2.0), `max_tokens`/`timeout` (≥1), `max_tokens_param`, `response_format`, `output.format`, `hook.mode`/`on_violation`/`min_severity`, and `provider.base_url` scheme. Multiple errors are aggregated into one message. +- **Profile values are validated (#81).** Focus area `weight` must be 1–10, and `action`/`tone`/`detail_level` must be recognized values; all built-in profiles pass. + +### Added — Config Safety + +- **`deny_unknown_fields` on all config sections (#80).** Misspelled YAML keys (e.g. `quailty_gate`, `temprature`) are now rejected at parse time instead of being silently dropped. + ## [0.6.2] - 2026-06-21 ### Fixed — Token Usage Tracking diff --git a/docs/changelog.md b/docs/changelog.md index 8247a3b..081f804 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — Config Validation & Best Practices (#334) + +- **Quality gate no longer fails when disabled (#58).** `evaluate()` now forces `GateResult::Pass` when `enabled: false` instead of applying thresholds. +- **Category actions are now validated enums (#57).** `quality_gate.categories.*.action` is a case-insensitive `CategoryAction` enum (`block`/`warn`/`ignore`); unknown values fail loudly at config load instead of silently becoming blocking. +- **Config values are validated (#94).** `Config::validate()` rejects out-of-range/unsupported values at load: `temperature` (0.0–2.0), `max_tokens`/`timeout` (≥1), `max_tokens_param`, `response_format`, `output.format`, `hook.*`, and `provider.base_url`. +- **Profile values are validated (#81).** Focus `weight` must be 1–10; `action`/`tone`/`detail_level` must be recognized. +- **`deny_unknown_fields` on all config sections (#80).** Misspelled YAML keys are rejected at parse time. + ## [0.6.2] - 2026-06-21 ### Fixed — Token Usage Tracking diff --git a/src/config/loader.rs b/src/config/loader.rs index 53543b0..7e168d2 100644 --- a/src/config/loader.rs +++ b/src/config/loader.rs @@ -299,6 +299,10 @@ pub fn load_config( config.output.color = false; } + // #94: validate semantic constraints after all sources are merged so typos + // and out-of-range values fail loudly at load instead of at runtime. + config.validate()?; + Ok(config) } diff --git a/src/config/schema.rs b/src/config/schema.rs index fba49e7..7b0a015 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -148,8 +148,121 @@ impl HookConfig { } } +impl Config { + /// Validate semantic constraints on resolved config values (#94). + /// + /// Catches typos and out-of-range values that would otherwise propagate + /// silently to runtime (e.g. `temperature: 5.0`, an unsupported output + /// format, or a misspelled `max_tokens_param`). Called after merging all + /// config sources in `loader::load_config`. + pub fn validate(&self) -> std::result::Result<(), CoraError> { + let mut errs: Vec = Vec::new(); + + // ── provider ── + if self.provider.provider.trim().is_empty() { + errs.push("provider.provider must not be empty".into()); + } + let base = self.provider.base_url.trim(); + let valid_scheme = base.is_empty() + || base.starts_with("http://") + || base.starts_with("https://") + || base.starts_with("ws://") + || base.starts_with("unix:"); + if !valid_scheme { + errs.push(format!( + "provider.base_url must be an http(s) URL, got: {}", + self.provider.base_url + )); + } + + // ── llm ── + if !(0.0..=2.0).contains(&self.temperature) { + errs.push(format!( + "llm.temperature must be between 0.0 and 2.0, got: {}", + self.temperature + )); + } + if self.max_tokens == 0 { + errs.push("llm.max_tokens must be at least 1".into()); + } + if self.timeout == 0 { + errs.push("llm.timeout must be at least 1 second".into()); + } + const VALID_TOKEN_PARAMS: &[&str] = &[ + "auto", + "max_tokens", + "max_output_tokens", + "max_completion_tokens", + ]; + if !VALID_TOKEN_PARAMS.contains(&self.max_tokens_param.as_str()) { + errs.push(format!( + "llm.max_tokens_param must be one of {:?}, got: {}", + VALID_TOKEN_PARAMS, self.max_tokens_param + )); + } + + // ── response format ── + const VALID_RESPONSE_FORMATS: &[&str] = &["none", "json_object"]; + if !VALID_RESPONSE_FORMATS.contains(&self.response_format.as_str()) { + errs.push(format!( + "review.response_format must be one of {:?}, got: {}", + VALID_RESPONSE_FORMATS, self.response_format + )); + } + + // ── output ── + const VALID_OUTPUT_FORMATS: &[&str] = &["pretty", "json", "compact", "sarif"]; + if !VALID_OUTPUT_FORMATS.contains(&self.output.format.as_str()) { + errs.push(format!( + "output.format must be one of {:?}, got: {}", + VALID_OUTPUT_FORMATS, self.output.format + )); + } + + // ── hook ── + const VALID_HOOK_MODES: &[&str] = &["warn", "block"]; + if !VALID_HOOK_MODES.contains(&self.hook.mode.as_str()) { + errs.push(format!( + "hook.mode must be one of {:?}, got: {}", + VALID_HOOK_MODES, self.hook.mode + )); + } + const VALID_VIOLATION: &[&str] = &["warn", "disallow"]; + if !VALID_VIOLATION.contains(&self.hook.on_violation.as_str()) { + errs.push(format!( + "hook.on_violation must be one of {:?}, got: {}", + VALID_VIOLATION, self.hook.on_violation + )); + } + const VALID_SEVERITIES: &[&str] = &["critical", "major", "minor", "info"]; + if !VALID_SEVERITIES.contains(&self.hook.min_severity.to_lowercase().as_str()) { + errs.push(format!( + "hook.min_severity must be one of {:?}, got: {}", + VALID_SEVERITIES, self.hook.min_severity + )); + } + + // ── profile (#81) ── + if let Some(profile) = &self.profile { + if let Err(pe) = profile.validate() { + errs.push(pe); + } + } + + if errs.is_empty() { + Ok(()) + } else { + Err(CoraError::ConfigParse(format!( + "invalid configuration:\n - {}", + errs.join("\n - ") + ))) + } + } +} + /// Serde-compatible schema for the `.cora.yaml` configuration file. #[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct CoraFile { #[serde( default, @@ -190,6 +303,7 @@ pub struct CoraFile { } #[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct ProviderSection { #[serde(skip_serializing_if = "Option::is_none")] pub provider: Option, @@ -223,6 +337,7 @@ where } #[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct IgnoreSection { #[serde(skip_serializing_if = "Option::is_none")] pub files: Option>, @@ -231,6 +346,7 @@ pub struct IgnoreSection { } #[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct HookSection { #[serde(skip_serializing_if = "Option::is_none")] pub mode: Option, @@ -243,6 +359,7 @@ pub struct HookSection { } #[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct OutputSection { #[serde(skip_serializing_if = "Option::is_none")] pub format: Option, @@ -252,6 +369,7 @@ pub struct OutputSection { /// Review-specific configuration section. #[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct ReviewSection { #[serde(skip_serializing_if = "Option::is_none")] pub response_format: Option, @@ -286,6 +404,7 @@ fn is_default(val: &T) -> bool { /// Scan-specific configuration section. #[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct ScanSection { #[serde(skip_serializing_if = "Option::is_none")] pub system_prompt: Option, @@ -295,6 +414,7 @@ pub struct ScanSection { /// LLM-specific configuration section (temperature, `max_tokens`, timeout, `cache_ttl`). #[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct LlmSection { #[serde(skip_serializing_if = "Option::is_none")] pub temperature: Option, @@ -317,6 +437,7 @@ fn default_max_findings() -> usize { /// Rule engine configuration section for `.cora.yaml`. #[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct RulesSection { #[serde(default, skip_serializing_if = "is_default")] pub enabled: bool, @@ -328,6 +449,7 @@ pub struct RulesSection { /// File bundling configuration section for `.cora.yaml`. #[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(deny_unknown_fields)] pub struct BundlingSection { #[serde(skip_serializing_if = "Option::is_none")] pub max_chars_per_group: Option, @@ -1345,4 +1467,80 @@ bundling: assert_eq!(back.max_tokens_param, Some("max_output_tokens".to_string())); assert_eq!(back.max_tokens, Some(8192)); } + + // ─── #94: Config::validate ─── + + #[test] + fn validate_accepts_default_config() { + assert!(Config::default().validate().is_ok()); + } + + #[test] + fn validate_rejects_out_of_range_temperature() { + let cfg = Config { + temperature: 5.0, + ..Default::default() + }; + let err = cfg.validate().unwrap_err().to_string(); + assert!(err.contains("temperature"), "err: {err}"); + } + + #[test] + fn validate_rejects_invalid_output_format() { + let mut cfg = Config::default(); + cfg.output.format = "prety".to_string(); // typo + let err = cfg.validate().unwrap_err().to_string(); + assert!(err.contains("output.format"), "err: {err}"); + } + + #[test] + fn validate_rejects_invalid_max_tokens_param() { + let cfg = Config { + max_tokens_param: "tokens".to_string(), + ..Default::default() + }; + assert!(cfg.validate().is_err()); + } + + #[test] + fn validate_rejects_invalid_base_url() { + let mut cfg = Config::default(); + cfg.provider.base_url = "api.openai.com".to_string(); // missing scheme + let err = cfg.validate().unwrap_err().to_string(); + assert!(err.contains("base_url"), "err: {err}"); + } + + #[test] + fn validate_aggregates_multiple_errors() { + let cfg = Config { + temperature: 9.0, + timeout: 0, + ..Default::default() + }; + let err = cfg.validate().unwrap_err().to_string(); + assert!(err.contains("temperature"), "err: {err}"); + assert!(err.contains("timeout"), "err: {err}"); + } + + // ─── #80: deny_unknown_fields ─── + + #[test] + fn deny_unknown_fields_rejects_top_level_typo() { + let yaml = "\nquailty_gate:\n enabled: true\n"; + let result = CoraFile::from_str(yaml); + assert!(result.is_err(), "misspelled top-level key must be rejected"); + } + + #[test] + fn deny_unknown_fields_rejects_section_typo() { + let yaml = "\nllm:\n temprature: 0.5\n"; + let result = CoraFile::from_str(yaml); + assert!(result.is_err(), "misspelled section key must be rejected"); + } + + #[test] + fn deny_unknown_fields_allows_valid_keys() { + let yaml = "\nllm:\n temperature: 0.5\n max_tokens: 8192\n"; + assert!(CoraFile::from_str(yaml).is_ok()); + } } diff --git a/src/engine/profiles.rs b/src/engine/profiles.rs index ef9911e..f2fb3aa 100644 --- a/src/engine/profiles.rs +++ b/src/engine/profiles.rs @@ -124,6 +124,54 @@ pub struct InlineProfileRef { // ─── Profile operations ─── +impl Profile { + /// Validate profile values (#81): focus weights must be 1-10, actions and + /// review-style enums must be recognized. Unknown values fail loudly so a + /// typo (e.g. `weight: 15` or `action: blok`) does not silently change + /// review behavior. + pub fn validate(&self) -> Result<(), String> { + const VALID_ACTIONS: &[&str] = &["block", "warn", "info"]; + const VALID_TONES: &[&str] = &["strict", "standard", "gentle"]; + const VALID_DETAILS: &[&str] = &["minimal", "standard", "high", "exhaustive"]; + let mut errs: Vec = Vec::new(); + + for area in &self.focus_areas { + if !(1..=10).contains(&area.weight) { + errs.push(format!( + "profile focus area '{}' weight must be 1-10, got: {}", + area.id, area.weight + )); + } + if !VALID_ACTIONS.contains(&area.action.as_str()) { + errs.push(format!( + "profile focus area '{}' action must be one of {:?}, got: '{}'", + area.id, VALID_ACTIONS, area.action + )); + } + } + + let style = &self.review_style; + if !style.tone.is_empty() && !VALID_TONES.contains(&style.tone.as_str()) { + errs.push(format!( + "profile review_style.tone must be one of {:?}, got: '{}'", + VALID_TONES, style.tone + )); + } + if !style.detail_level.is_empty() && !VALID_DETAILS.contains(&style.detail_level.as_str()) { + errs.push(format!( + "profile review_style.detail_level must be one of {:?}, got: '{}'", + VALID_DETAILS, style.detail_level + )); + } + + if errs.is_empty() { + Ok(()) + } else { + Err(errs.join("; ")) + } + } +} + /// Load a built-in profile by name. Returns `None` if not found. pub fn load_builtin(name: &str) -> Option { let yaml = match name { @@ -460,4 +508,61 @@ mod tests { assert_eq!(back.name, p.name); assert_eq!(back.focus_areas.len(), p.focus_areas.len()); } + + // ─── #81: Profile::validate ─── + + #[test] + fn validate_accepts_all_builtins() { + for name in BUILTIN_PROFILES { + let p = load_builtin(name).unwrap_or_else(|| panic!("missing builtin {name}")); + assert!(p.validate().is_ok(), "builtin '{name}' failed validation"); + } + } + + #[test] + fn validate_rejects_weight_out_of_range() { + let p = Profile { + name: "test".into(), + focus_areas: vec![FocusArea { + id: "x".into(), + weight: 15, + action: "warn".into(), + rules: vec![], + }], + ..Default::default() + }; + let err = p.validate().unwrap_err(); + assert!(err.contains("weight"), "err: {err}"); + } + + #[test] + fn validate_rejects_unknown_action() { + let p = Profile { + name: "test".into(), + focus_areas: vec![FocusArea { + id: "x".into(), + weight: 5, + action: "blok".into(), // typo + rules: vec![], + }], + ..Default::default() + }; + let err = p.validate().unwrap_err(); + assert!(err.contains("action"), "err: {err}"); + } + + #[test] + fn validate_rejects_unknown_tone() { + let p = Profile { + name: "test".into(), + review_style: ReviewStyle { + tone: "angry".into(), + detail_level: "standard".into(), + suggest_fixes: true, + max_findings: None, + }, + ..Default::default() + }; + assert!(p.validate().is_err()); + } } diff --git a/src/engine/quality_gate.rs b/src/engine/quality_gate.rs index 190d2ff..86798b7 100644 --- a/src/engine/quality_gate.rs +++ b/src/engine/quality_gate.rs @@ -10,6 +10,7 @@ use std::collections::HashMap; /// Quality gate configuration — parsed from `.cora.yaml` under `quality_gate`. #[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct QualityGateConfig { /// Enable quality gate evaluation. #[serde(default)] @@ -26,6 +27,7 @@ pub struct QualityGateConfig { /// Global threshold configuration. #[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct ThresholdConfig { /// Max critical issues allowed (default: 0). #[serde(default = "default_max_critical")] @@ -67,22 +69,64 @@ impl Default for ThresholdConfig { } } +/// Per-category gate action. +/// +/// Deserialized case-insensitively from `.cora.yaml` so that `block`, +/// `Block`, and `BLOCK` are all accepted — but an unknown value like `blok` +/// fails loudly at config load instead of silently becoming blocking (#57). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Default)] +#[serde(rename_all = "lowercase")] +pub enum CategoryAction { + Block, + #[default] + Warn, + Ignore, +} + +impl CategoryAction { + pub fn as_str(self) -> &'static str { + match self { + CategoryAction::Block => "block", + CategoryAction::Warn => "warn", + CategoryAction::Ignore => "ignore", + } + } + + /// Parse case-insensitively. Returns `None` for unknown values. + pub fn from_str_lossy(s: &str) -> Option { + match s.to_ascii_lowercase().as_str() { + "block" => Some(Self::Block), + "warn" => Some(Self::Warn), + "ignore" => Some(Self::Ignore), + _ => None, + } + } +} + +impl<'de> Deserialize<'de> for CategoryAction { + fn deserialize(deserializer: D) -> std::result::Result + where + D: serde::Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + Self::from_str_lossy(&s) + .ok_or_else(|| serde::de::Error::unknown_variant(&s, &["block", "warn", "ignore"])) + } +} + /// Per-category gate configuration. #[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct CategoryConfig { - /// Action: "block" (fail CI), "warn" (comment only), "ignore" (skip). - #[serde(default = "default_category_action")] - pub action: String, + /// Action: `block` (fail CI), `warn` (comment only), `ignore` (skip). + #[serde(default)] + pub action: CategoryAction, /// Max findings allowed in this category. #[serde(default = "default_disabled")] pub max_findings: usize, } -fn default_category_action() -> String { - "warn".to_string() -} - /// Result of quality gate evaluation. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct GateResult { @@ -131,7 +175,7 @@ pub struct CheckResult { /// Did it pass? pub passed: bool, /// Category action (for category checks). - pub action: Option, + pub action: Option, } /// Counts of findings by severity. @@ -212,7 +256,7 @@ pub fn evaluate(issues: &[ReviewIssue], config: &QualityGateConfig) -> GateResul // Evaluate per-category overrides for (category, cat_config) in &config.categories { - if cat_config.action == "ignore" { + if cat_config.action == CategoryAction::Ignore { continue; } let cat_count = category_counts.get(category).copied().unwrap_or(0); @@ -221,15 +265,21 @@ pub fn evaluate(issues: &[ReviewIssue], config: &QualityGateConfig) -> GateResul threshold: cat_config.max_findings, actual: cat_count, passed: cat_count <= cat_config.max_findings, - action: Some(cat_config.action.clone()), + action: Some(cat_config.action), }); } // Determine overall status: - // FAIL if any "block" category or global threshold is exceeded - let status = if checks + // #58: A disabled gate must never FAIL. Counts and checks are still + // produced for reporting, but a disabled gate forces PASS. + // Otherwise FAIL if any "block" category or global threshold is exceeded. + // Global thresholds have action None (always fail when exceeded); + // "warn" categories never fail the gate. + let status = if !config.enabled { + GateStatus::Pass + } else if checks .iter() - .any(|c| !c.passed && c.action.as_deref() != Some("warn")) + .any(|c| !c.passed && c.action != Some(CategoryAction::Warn)) { GateStatus::Fail } else { @@ -286,8 +336,7 @@ pub fn format_gate_output(result: &GateResult) -> String { }; let action_label = check .action - .as_deref() - .map(|a| format!(" ({})", a.dimmed())) + .map(|a| format!(" ({})", a.as_str().dimmed())) .unwrap_or_default(); out.push_str(&format!( " {} {:<20} → {} found {}{}\n", @@ -377,7 +426,7 @@ mod tests { categories.insert( "performance".to_string(), CategoryConfig { - action: "block".to_string(), + action: CategoryAction::Block, max_findings: 0, }, ); @@ -397,7 +446,7 @@ mod tests { categories.insert( "performance".to_string(), CategoryConfig { - action: "warn".to_string(), + action: CategoryAction::Warn, max_findings: 0, }, ); @@ -418,7 +467,7 @@ mod tests { categories.insert( "style".to_string(), CategoryConfig { - action: "ignore".to_string(), + action: CategoryAction::Ignore, max_findings: 0, }, ); @@ -509,4 +558,47 @@ mod tests { assert!(!major_check.passed); assert_eq!(major_check.actual, 2); } + + // ─── #58: disabled gate must never fail ─── + + #[test] + fn gate_disabled_never_fails_even_with_critical() { + let issues = vec![ + make_issue(Severity::Critical, "security"), + make_issue(Severity::Critical, "bug"), + ]; + let config = QualityGateConfig { + enabled: false, + ..Default::default() + }; + let result = evaluate(&issues, &config); + assert_eq!(result.status, GateStatus::Pass); + // No threshold checks are produced when disabled. + assert!(!result.checks.is_empty()); + // total_findings still reflects input for reporting. + assert_eq!(result.total_findings, 2); + // Counts are still computed for reporting even when the gate is off. + assert_eq!(result.severity_counts.critical, 2); + } + + // ─── #57: CategoryAction case-insensitive enum ─── + + #[test] + fn category_action_deserializes_case_insensitive() { + let yaml = "action: BLOCK\nmax_findings: 0"; + let cc: CategoryConfig = serde_yaml_ng::from_str(yaml).unwrap(); + assert_eq!(cc.action, CategoryAction::Block); + + let yaml2 = "action: Warn"; + let cc2: CategoryConfig = serde_yaml_ng::from_str(yaml2).unwrap(); + assert_eq!(cc2.action, CategoryAction::Warn); + } + + #[test] + fn category_action_unknown_value_errors() { + let yaml = "action: blok\nmax_findings: 0"; + let result: Result = serde_yaml_ng::from_str(yaml); + // A typo must fail loudly, not silently become blocking. + assert!(result.is_err()); + } }