From 48b94d33efbbd4fe1b36350ec63c45cd4b2e6a52 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Fri, 16 Oct 2026 16:34:14 +0700 Subject: [PATCH] fix(config): validate custom rules, accept language names, add a dry run (#684, #678) Signed-off-by: ajianaz --- CHANGELOG.md | 3 +- docs/configuration.md | 42 +++++++++----- src/commands/config_cmd.rs | 51 +++++++++++++++++ src/config/schema.rs | 5 ++ src/engine/rules/matching.rs | 50 +++++++++++++++-- src/engine/rules/mod.rs | 104 ++++++++++++++++++++++++++++++++++- src/engine/rules/types.rs | 11 +++- src/main.rs | 22 +++++++- tests/custom_rules.rs | 92 +++++++++++++++++++++++++++++++ 9 files changed, 354 insertions(+), 26 deletions(-) create mode 100644 tests/custom_rules.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d27a954..e9ebcd59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Custom rules are validated and the docs match the schema (#684, #678).** `cora config validate` and config load now reject an invalid regex, an empty or duplicate id, an id that shadows a built-in rule, an empty message and an unknown `languages` value, naming the rule. `languages` accepts language names (`"rust"`) as well as extensions (`"rs"`); `languages` and `exclude` are optional. `cora config validate --dry-run [--path DIR]` runs each custom rule and prints how many lines it matched. `docs/configuration.md` now shows the real schema (`rules_engine.custom`); the 0.19.0 entry no longer says `custom_rules`. - The `Cora Review` workflow now fails with an error naming each empty secret (`CORA_API_KEY`, `CORA_BASE_URL`, `CORA_MODEL`) instead of running without them. - The `Cora Review` job no longer gets cancelled on larger pull requests: its time limit is 30 minutes (the review action allows up to three LLM attempts of 10 minutes each). @@ -66,7 +67,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **The MCP `cora.review_diff` tool no longer crashes the server (#647).** Every call panicked with "Cannot start a runtime from within a runtime" because the handler blocked on a second runtime from a tokio runtime thread. Handlers now go through `block_in_place`. -- **`cora scan` now runs the rules engine (#645).** Built-in rules (`bug-todo`, `sec-sql-concat`, ...) and custom `rules_engine.custom_rules` were only run by `cora review`; scan silently skipped them. Expect more findings from `cora scan` after upgrading (e.g. `bug-todo` on every TODO), still under the `rules_engine.max_findings` cap (default 5); `rules_engine.enabled: false` turns them off. One set of deterministic families (`deterministic::run_files` for whole files, `deterministic::run` for diffs) now serves both, and the MCP `cora.check_snippet` tool picks up rules too. Scan also has a judge seam (`judge::ScanJudge`, with `HttpScanJudge` and `NoJudge`) so the batch loop is tested without a network, and `DEFAULT_BATCH_TOKEN_BUDGET` is renamed `DEFAULT_BATCH_CHAR_BUDGET` (it was always a character count). `docs/cli-reference.md` now states which review flags scan does not have. +- **`cora scan` now runs the rules engine (#645).** Built-in rules (`bug-todo`, `sec-sql-concat`, ...) and custom `rules_engine.custom` rules were only run by `cora review`; scan silently skipped them. Expect more findings from `cora scan` after upgrading (e.g. `bug-todo` on every TODO), still under the `rules_engine.max_findings` cap (default 5); `rules_engine.enabled: false` turns them off. One set of deterministic families (`deterministic::run_files` for whole files, `deterministic::run` for diffs) now serves both, and the MCP `cora.check_snippet` tool picks up rules too. Scan also has a judge seam (`judge::ScanJudge`, with `HttpScanJudge` and `NoJudge`) so the batch loop is tested without a network, and `DEFAULT_BATCH_TOKEN_BUDGET` is renamed `DEFAULT_BATCH_CHAR_BUDGET` (it was always a character count). `docs/cli-reference.md` now states which review flags scan does not have. ## [0.18.0] - 2026-10-09 diff --git a/docs/configuration.md b/docs/configuration.md index b59bc055..1250caa8 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -437,25 +437,37 @@ profile: security-first ## Custom Rule Engine -Write your own regex-based rules in `.cora.yaml`: +Write your own regex-based rules under `rules_engine.custom` in `.cora.yaml`: ```yaml -rules: - - id: no-unwrap - pattern: "\\.unwrap\\(\\)" - severity: minor - message: "Avoid unwrap() in production code — use proper error handling" - languages: ["rust"] - exclude: ["tests/**"] - - - id: no-console-log - pattern: "console\\.log\\(" - severity: minor - message: "Remove console.log before merging" - languages: ["typescript", "javascript"] +rules_engine: + custom: + - id: no-unwrap + pattern: "\\.unwrap\\(\\)" + severity: minor + message: "Avoid unwrap() in production code — use proper error handling" + languages: ["rs"] + exclude: ["tests/**"] + + - id: no-console-log + pattern: "console\\.log\\(" + severity: minor + message: "Remove console.log before merging" + languages: ["ts", "js"] ``` -Rules run during the deterministic pre-scan phase (no LLM needed). +| Field | Required | Description | +|-------|----------|-------------| +| `id` | yes | Unique rule id; must not collide with a built-in rule id (it is what `ignore.rules` and `cora-ignore` name) | +| `pattern` | yes | A regex matched against each line; an invalid regex is a config error | +| `severity` | yes | `critical`, `major`, `minor` or `info` | +| `message` | yes | The finding text | +| `languages` | no (default `["all"]`) | File extensions (`"rs"`, `"py"`, `"ts"`) or language names (`"rust"`, `"typescript"`); `"all"` or `"*"` matches every file. An unknown value is a config error | +| `exclude` | no (default none) | Glob patterns of paths to skip | + +The top-level `rules:` key is a different thing: a list of free-text instructions for the LLM reviewer (strings), not rule objects. + +Rules run during the deterministic pre-scan phase (no LLM needed). `cora config validate` rejects an invalid pattern, a duplicate id, an unknown language or an empty message with the rule id. `cora config validate --dry-run [--path DIR]` also runs every custom rule over the project and prints how many lines each one matched, so a rule that matches nothing is visible. ### Index Scanner Configuration diff --git a/src/commands/config_cmd.rs b/src/commands/config_cmd.rs index fa25b610..6de32a8a 100644 --- a/src/commands/config_cmd.rs +++ b/src/commands/config_cmd.rs @@ -439,6 +439,57 @@ pub fn execute_config_set(key: &str, value: &str, global: bool) -> Result<()> { Ok(()) } +/// Execute `cora config validate --dry-run`: run every custom rule over the +/// project and print how many lines each one matched (#684). A rule that +/// matches nothing is reported, since that is how a wrong pattern shows up. +pub fn execute_config_dry_run(config_path: Option<&str>, path: Option<&str>) -> Result { + use crate::engine::rules; + use crate::engine::scan_input; + + let config = loader::load_config(config_path, None, None, None, None, false)?; + let custom = &config.rules_config.custom_rules; + if custom.is_empty() { + println!("No custom rules configured (rules_engine.custom)."); + return Ok(0); + } + let root = match path { + Some(p) => PathBuf::from(p), + None => std::env::current_dir()?, + }; + let files = crate::engine::scanner::walk_project(&root, &[], &config.ignore.files, &[]) + .map_err(|e| anyhow::anyhow!("{e}"))?; + let mut only_custom = config.rules_config.clone(); + only_custom.include_tests = true; + only_custom.max_findings = 0; + let findings = rules::run_rules_in(&scan_input::from_entries(&files), &only_custom); + println!("Dry run over {} files in {}", files.len(), root.display()); + for rule in custom { + let hits: Vec<_> = findings.iter().filter(|f| f.rule_id == rule.id).collect(); + let n_files = hits + .iter() + .map(|f| f.file.as_str()) + .collect::>() + .len(); + if hits.is_empty() { + println!( + "{} {}: 0 matches. Check the pattern and `languages` ({:?})", + "⚠️".yellow(), + rule.id, + rule.languages + ); + } else { + println!( + "{} {}: {} lines in {} files", + "✅".green(), + rule.id, + hits.len(), + n_files + ); + } + } + Ok(0) +} + /// Execute `cora config validate` — load config and report validity. /// /// Returns exit code: 0 if valid, 2 if issues found. diff --git a/src/config/schema.rs b/src/config/schema.rs index e59df2b1..9eb333f0 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -296,6 +296,11 @@ impl Config { )); } + // ── custom rules (#684) ── + errs.extend(crate::engine::rules::validate_custom_rules( + &self.rules_config.custom_rules, + )); + // ── profile (#81) ── if let Some(profile) = &self.profile { if let Err(pe) = profile.validate() { diff --git a/src/engine/rules/matching.rs b/src/engine/rules/matching.rs index 076a0b55..d9c5d6b4 100644 --- a/src/engine/rules/matching.rs +++ b/src/engine/rules/matching.rs @@ -5,10 +5,52 @@ use super::types::CustomRule; /// Check if a rule applies to the given file language. pub fn matches_language(rule: &CustomRule, file_lang: &str) -> bool { - rule.languages.iter().any(|lang| { - let lang_lower = lang.to_lowercase(); - lang_lower == "all" || lang_lower == file_lang - }) + rule.languages + .iter() + .any(|lang| match normalize_language(lang) { + Some(n) => { + // A scan sees the raw extension (`tsx`), a diff the shorthand (`ts`). + n == "all" + || n == file_lang + || normalize_language(file_lang).as_deref() == Some(n.as_str()) + } + // Unknown value: keep the old literal comparison (config validation reports it). + None => lang.to_lowercase() == file_lang, + }) +} + +/// Map a configured language (extension or name) to the shorthand used for +/// files (`"rust"` and `"rs"` -> `"rs"`). `"all"` / `"*"` -> `"all"`. +/// `None` when it names no known language. +pub fn normalize_language(lang: &str) -> Option { + let l = lang.trim().trim_start_matches('.').to_lowercase(); + if l == "all" || l == "*" { + return Some("all".to_string()); + } + let by_name = match l.as_str() { + "rust" => "rs", + "python" => "py", + "typescript" => "ts", + "javascript" | "node" => "js", + "golang" => "go", + "kotlin" => "kt", + "ruby" => "rb", + "shell" | "bash" => "sh", + "markdown" => "md", + "csharp" | "c#" => "cs", + "c++" => "cpp", + "elixir" => "ex", + _ => "", + }; + if !by_name.is_empty() { + return Some(by_name.to_string()); + } + let detected = crate::engine::diff_parser::detect_language(&format!("x.{l}")); + if detected == "unknown" { + // Languages detect_language does not know but that are real extensions. + return matches!(l.as_str(), "cs" | "vue" | "tf" | "xml" | "ini" | "txt").then_some(l); + } + Some(detected.to_string()) } /// Check if a file path should be excluded from a rule based on glob patterns. diff --git a/src/engine/rules/mod.rs b/src/engine/rules/mod.rs index 74a29d61..3eed6042 100644 --- a/src/engine/rules/mod.rs +++ b/src/engine/rules/mod.rs @@ -7,7 +7,7 @@ pub mod types; use tracing::debug; use crate::engine::diff_parser::{FileChunk, parse_diff}; -use crate::engine::rules::types::{RuleFinding, RulesConfig}; +use crate::engine::rules::types::{CustomRule, RuleFinding, RulesConfig}; use crate::engine::scan_input::{self, ScanFile}; use crate::engine::types::{ReviewIssue, Severity}; @@ -45,6 +45,51 @@ const TEST_EXEMPT_RULES: &[&str] = &[ "sec-hardcoded-secret", ]; +/// Lint custom rules: returns one message per problem (empty = fine). +/// +/// Catches what would otherwise fail silently: a pattern that is not a valid +/// regex, an id that is empty, duplicated or shadows a built-in rule, an empty +/// message, and `languages` values that match no file (#684, #678). +pub fn validate_custom_rules(rules: &[CustomRule]) -> Vec { + let builtin_ids: std::collections::HashSet = + builtin::builtin_rules().into_iter().map(|r| r.id).collect(); + let mut seen = std::collections::HashSet::new(); + let mut errs = Vec::new(); + for (i, r) in rules.iter().enumerate() { + let name = if r.id.is_empty() { + format!("rules_engine.custom[{i}]") + } else { + format!("rules_engine.custom[{i}] ({})", r.id) + }; + if r.id.trim().is_empty() { + errs.push(format!("{name}: id must not be empty")); + } else if builtin_ids.contains(&r.id) { + errs.push(format!("{name}: id collides with a built-in rule")); + } else if !seen.insert(r.id.clone()) { + errs.push(format!("{name}: duplicate id")); + } + if let Err(e) = regex::Regex::new(&r.pattern) { + errs.push(format!("{name}: pattern is not a valid regex: {e}")); + } + if r.message.trim().is_empty() { + errs.push(format!("{name}: message must not be empty")); + } + if r.languages.is_empty() { + errs.push(format!( + "{name}: languages is empty, so the rule can never match; use [\"all\"] or file extensions like [\"rs\"]" + )); + } + for lang in &r.languages { + if matching::normalize_language(lang).is_none() { + errs.push(format!( + "{name}: languages entry \"{lang}\" is not a known extension or language (use e.g. \"rs\", \"py\", \"ts\", or \"all\")" + )); + } + } + } + errs +} + pub fn run_rules_in(files: &[ScanFile<'_>], config: &RulesConfig) -> Vec { if !config.enabled { debug!("rule engine is disabled"); @@ -317,6 +362,63 @@ mod tests { use super::*; use crate::engine::Severity; + fn rule(id: &str, pattern: &str, languages: &[&str]) -> CustomRule { + CustomRule { + id: id.into(), + pattern: pattern.into(), + severity: Severity::Major, + message: "m".into(), + languages: languages.iter().map(|s| s.to_string()).collect(), + exclude: vec![], + compiled_pattern: None, + } + } + + #[test] + fn a_valid_custom_rule_has_no_problems() { + assert!(validate_custom_rules(&[rule("mine", "fn a", &["rs"])]).is_empty()); + assert!( + validate_custom_rules(&[rule("mine", "x", &["rust", "TypeScript", "*"])]).is_empty() + ); + } + + #[test] + fn validation_reports_each_silent_failure_with_the_rule_id() { + let errs = validate_custom_rules(&[ + rule("bad", "(", &["rs"]), + rule("lang", "x", &["klingon"]), + rule("lang", "x", &[]), + rule("bug-todo", "x", &["rs"]), + ]); + let all = errs.join("\n"); + assert!( + all.contains("(bad)") && all.contains("not a valid regex"), + "{all}" + ); + assert!(all.contains("klingon"), "{all}"); + assert!(all.contains("duplicate id"), "{all}"); + assert!(all.contains("languages is empty"), "{all}"); + assert!(all.contains("collides with a built-in rule"), "{all}"); + } + + #[test] + fn language_names_match_files_like_extensions() { + let r = rule("mine", "fn a", &["rust"]); + assert!(matching::matches_language(&r, "rs")); + assert!(!matching::matches_language(&r, "py")); + assert!(matching::matches_language(&rule("m", "x", &["tsx"]), "ts")); + } + + #[test] + fn exclude_and_languages_are_optional_in_yaml() { + let r: CustomRule = serde_yaml_ng::from_str( + "id: no-console-log\npattern: 'console'\nseverity: minor\nmessage: no\n", + ) + .unwrap(); + assert!(r.exclude.is_empty()); + assert_eq!(r.languages, vec!["all".to_string()]); + } + fn default_config() -> RulesConfig { RulesConfig { enabled: true, diff --git a/src/engine/rules/types.rs b/src/engine/rules/types.rs index 2827db75..bb644565 100644 --- a/src/engine/rules/types.rs +++ b/src/engine/rules/types.rs @@ -90,15 +90,22 @@ pub struct CustomRule { pub severity: Severity, /// Human-readable description used as the finding body. pub message: String, - /// Languages this rule applies to. `["all"]` means all languages. + /// Languages this rule applies to: file extensions (`"rs"`) or language + /// names (`"rust"`). `["all"]` (or `"*"`) means all languages; omitted = all. + #[serde(default = "default_languages")] pub languages: Vec, - /// Glob patterns for file paths to exclude from this rule. + /// Glob patterns for file paths to exclude from this rule (default: none). + #[serde(default)] pub exclude: Vec, /// Pre-compiled regex for the pattern (populated after rule assembly). #[serde(skip, default)] pub compiled_pattern: Option, } +fn default_languages() -> Vec { + vec!["all".to_string()] +} + impl CustomRule { /// Compile the rule's pattern into a cached regex, if not already compiled. /// Returns `true` if compilation succeeded, `false` if the pattern is invalid. diff --git a/src/main.rs b/src/main.rs index dc3bfc2f..83afe22d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -670,7 +670,15 @@ enum ConfigAction { global: bool, }, /// Validate the current configuration and report status - Validate, + Validate { + /// Also run each custom rule over the project and report how many + /// lines it matched (a rule with 0 matches is flagged) + #[clap(long)] + dry_run: bool, + /// Directory for `--dry-run` (default: current directory) + #[clap(long, requires = "dry_run")] + path: Option, + }, } #[derive(Subcommand, Debug)] @@ -1331,8 +1339,16 @@ async fn main() -> Result<()> { config_cmd::execute_config_set(&key, &value, global)?; 0 } - ConfigAction::Validate => { - config_cmd::execute_config_validate(cli.global.config.as_deref())? + ConfigAction::Validate { dry_run, path } => { + let code = config_cmd::execute_config_validate(cli.global.config.as_deref())?; + if dry_run && code == 0 { + config_cmd::execute_config_dry_run( + cli.global.config.as_deref(), + path.as_deref(), + )? + } else { + code + } } }, Command::Providers => { diff --git a/tests/custom_rules.rs b/tests/custom_rules.rs new file mode 100644 index 00000000..59646b16 --- /dev/null +++ b/tests/custom_rules.rs @@ -0,0 +1,92 @@ +//! `cora config validate` must not call a broken custom rule valid (#684, #678). +use std::process::Command; + +fn cora(dir: &std::path::Path, args: &[&str]) -> std::process::Output { + Command::new(env!("CARGO_BIN_EXE_cora")) + .args(args) + .current_dir(dir) + .env_clear() + .env("PATH", "/usr/bin:/bin") + .env("HOME", dir) + .env("CORA_API_KEY", "test-key-not-real") + .output() + .unwrap() +} + +fn write(dir: &std::path::Path, name: &str, body: &str) { + if let Some(p) = std::path::Path::new(name).parent() { + std::fs::create_dir_all(dir.join(p)).unwrap(); + } + std::fs::write(dir.join(name), body).unwrap(); +} + +#[test] +fn an_invalid_regex_fails_validation_and_names_the_rule() { + let d = tempfile::tempdir().unwrap(); + write( + d.path(), + "bad.yaml", + "rules_engine:\n custom:\n - id: bad\n pattern: \"(\"\n severity: major\n message: m\n languages: [\"rs\"]\n exclude: []\n", + ); + let out = cora(d.path(), &["config", "validate", "--config", "bad.yaml"]); + assert!(!out.status.success(), "must not exit 0"); + let text = + String::from_utf8_lossy(&out.stderr).to_string() + &String::from_utf8_lossy(&out.stdout); + assert!(text.contains("bad") && text.contains("regex"), "{text}"); +} + +#[test] +fn a_config_copied_from_the_docs_loads_and_the_rule_fires() { + let d = tempfile::tempdir().unwrap(); + write( + d.path(), + ".cora.yaml", + "rules_engine:\n custom:\n - id: probe\n pattern: \"fn a\"\n severity: major\n message: probe\n languages: [\"rust\"]\n", + ); + write(d.path(), "src/a.rs", "fn a() {}\n"); + let ok = cora(d.path(), &["config", "validate"]); + assert!( + ok.status.success(), + "{}", + String::from_utf8_lossy(&ok.stderr) + ); + let scan = cora(d.path(), &["scan", "--no-llm", "--format", "compact"]); + let text = + String::from_utf8_lossy(&scan.stdout).to_string() + &String::from_utf8_lossy(&scan.stderr); + assert!(text.contains("probe"), "rule must fire: {text}"); +} + +#[test] +fn an_unknown_language_is_rejected_with_the_value() { + let d = tempfile::tempdir().unwrap(); + write( + d.path(), + ".cora.yaml", + "rules_engine:\n custom:\n - id: probe\n pattern: \"x\"\n severity: major\n message: m\n languages: [\"klingon\"]\n", + ); + let out = cora(d.path(), &["config", "validate"]); + assert!(!out.status.success()); + let text = + String::from_utf8_lossy(&out.stderr).to_string() + &String::from_utf8_lossy(&out.stdout); + assert!(text.contains("klingon"), "{text}"); +} + +#[test] +fn the_dry_run_flags_a_rule_that_matches_nothing() { + let d = tempfile::tempdir().unwrap(); + write( + d.path(), + ".cora.yaml", + "rules_engine:\n custom:\n - id: hit\n pattern: \"fn a\"\n severity: major\n message: m\n languages: [\"rs\"]\n - id: miss\n pattern: \"never_present_zzz\"\n severity: major\n message: m\n languages: [\"rs\"]\n", + ); + write(d.path(), "src/a.rs", "fn a() {}\n"); + let out = cora(d.path(), &["config", "validate", "--dry-run"]); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + let text = String::from_utf8_lossy(&out.stdout).to_string(); + assert!(text.contains("hit: 1 lines"), "{text}"); + assert!(text.contains("miss: 0 matches"), "{text}"); +}