From 0e3641229170288f9878c2f651ba6f0d14c85dd7 Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Sat, 27 Jun 2026 13:00:13 +0700 Subject: [PATCH 01/16] fix(review): add post-LLM filter for hardcoded secret false positives (#325) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(review): add post-LLM filter for hardcoded secret false positives Cora LLM sometimes flags struct field declarations (e.g. api_key: String) as 'Hardcoded password or secret in variable' even when no literal value is present. The built-in sec-hardcoded-secret regex only matches actual assignments like api_key = "sk-...", but the LLM can hallucinate these findings regardless of .cora.yaml rules. Add apply_llm_secret_fp_filter that cross-validates LLM security findings against actual diff lines. If a finding about hardcoded secrets points to a line that doesn't match the sec-hardcoded-secret regex, it's removed as a false positive. Unknown/unresolvable lines are kept (safe default). 4 unit tests cover: FP removal, real secret retention, non-security pass-through, and unknown-line preservation. * fix(deps): bump git2 0.20 → 0.21 to resolve RUSTSEC advisories - git2 0.21.0 fixes RUSTSEC-2026-0183 (null ptr in Remote::list) and RUSTSEC-2026-0184 (null ptr in Blame::blame_buffer Signature) - Remote::url() API changed from Option<&str> to Result<&str, Error> - Adapted get_repo_info() to use .url().ok() chain * fix(deps): bump quinn-proto 0.11.14 → 0.11.15 (RUSTSEC-2026-0185) Resolves remote memory exhaustion DoS vulnerability in quinn-proto unbounded out-of-order stream reassembly. --------- Co-authored-by: ajianaz --- Cargo.lock | 9 +- Cargo.toml | 2 +- src/engine/review.rs | 268 +++++++++++++++++++++++++++++++++++++++++++ src/git/diff.rs | 2 +- 4 files changed, 274 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0d83ca1..d1496d7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -599,15 +599,14 @@ dependencies = [ [[package]] name = "git2" -version = "0.20.4" +version = "0.21.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7b88256088d75a56f8ecfa070513a775dd9107f6530ef14919dac831af9cfe2b" +checksum = "ddddbf932745a6be37109b6112d3ee09696106f848449069d3a57bba937ab82e" dependencies = [ "bitflags", "libc", "libgit2-sys", "log", - "url", ] [[package]] @@ -1246,9 +1245,9 @@ dependencies = [ [[package]] name = "quinn-proto" -version = "0.11.14" +version = "0.11.15" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "434b42fec591c96ef50e21e886936e66d3cc3f737104fdb9b737c40ffb94c098" +checksum = "4fcb935c5bec503c2f0e306bdd3e58bb9029dcb14fa8d9ac76e3a5256ac0763e" dependencies = [ "bytes", "getrandom 0.3.4", diff --git a/Cargo.toml b/Cargo.toml index 5a59478..17e16a0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,7 +27,7 @@ serde_json = "1" serde_yaml_ng = "0.10" # Git operations -git2 = { version = "0.20", default-features = false, features = ["vendored-libgit2"] } +git2 = { version = "0.21", default-features = false, features = ["vendored-libgit2"] } # Terminal output colored = "3" diff --git a/src/engine/review.rs b/src/engine/review.rs index 81b8c7c..db0ae5f 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -329,6 +329,14 @@ async fn review_diff_inner( } } + // Cross-validate LLM security findings about hardcoded secrets against + // actual diff lines. The LLM sometimes flags struct field declarations + // (e.g. `api_key: String`) as "hardcoded secret" even when no literal + // value is present. This filter removes such false positives by checking + // the added line at the reported file:line against the built-in + // sec-hardcoded-secret regex. + response.issues = apply_llm_secret_fp_filter(response.issues, &diff_chunks); + // Apply ignore rules: filter out issues matching ignored patterns response.issues = apply_ignore_rules(response.issues, &config.ignore.rules); @@ -362,6 +370,108 @@ async fn review_diff_inner( Ok(response) } +/// Filter out LLM findings about hardcoded secrets/passwords that point to +/// diff lines which don't actually contain a literal string assignment. +/// +/// The LLM sometimes flags struct field declarations like `api_key: String` +/// or `api_key: extract_api_key.clone()` as "Hardcoded password or secret in +/// variable". These are identifiers, not hardcoded values. +/// +/// This function cross-validates each security finding against the actual +/// added line in the diff. If the line doesn't match the `sec-hardcoded-secret` +/// regex (i.e. no `password/key/secret = "literal"` pattern), the finding is +/// removed as a false positive. +fn apply_llm_secret_fp_filter( + mut issues: Vec, + diff_chunks: &[crate::engine::diff_parser::FileChunk], +) -> Vec { + use crate::engine::diff_parser::DiffLineType; + + // Lazy-compiled regex matching the built-in sec-hardcoded-secret pattern. + // Only triggers for actual value assignments like `api_key = "sk-..."`. + static RE_SECRET_LITERAL: std::sync::LazyLock = std::sync::LazyLock::new(|| { + regex::Regex::new(r#"(?i)(?:password|api_?key|token|secret)\s*=\s*"[^"]+""#) + .expect("hardcoded secret regex must compile") + }); + + // Keywords that indicate an LLM finding is about hardcoded secrets. + static SECRET_KEYWORDS: &[&str] = &[ + "hardcoded password", + "hardcoded secret", + "hardcoded credential", + "hardcoded token", + "hardcoded api key", + "hardcoded api_key", + ]; + + // Pre-compute a lookup: (file_path, new_line_no) -> line content + let added_lines: std::collections::HashMap<(String, u32), &str> = diff_chunks + .iter() + .flat_map(|chunk| { + let path = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or("unknown"); + chunk.chunks.iter().flat_map(|hunk| { + hunk.lines + .iter() + .filter(|l| l.line_type == DiffLineType::Add) + .filter_map(|l| { + l.new_line_no + .map(|ln| ((path.to_string(), ln), l.content.as_str())) + }) + }) + }) + .collect(); + + let before = issues.len(); + issues.retain(|issue| { + // Only check security-type findings about secrets + let issue_type = issue.issue_type.as_deref().unwrap_or(""); + let title_lower = issue.title.to_lowercase(); + + if issue_type != "security" { + return true; + } + + let is_secret_finding = SECRET_KEYWORDS.iter().any(|kw| title_lower.contains(kw)); + if !is_secret_finding { + return true; + } + + // Look up the actual diff line + let line_num = issue.line.unwrap_or(0); + let key = (issue.file.clone(), line_num); + if let Some(actual_line) = added_lines.get(&key) { + if !RE_SECRET_LITERAL.is_match(actual_line) { + debug!( + file = %issue.file, + line = line_num, + title = %issue.title, + "suppressed LLM false positive: line has no hardcoded secret literal" + ); + return false; // Remove this finding + } + } + + // If we can't find the line (hallucinated path/line or context line), + // keep the finding — better safe than sorry. + true + }); + + let filtered = before - issues.len(); + if filtered > 0 { + debug!( + filtered, + remaining = issues.len(), + "filtered LLM false positives for hardcoded secret findings" + ); + } + + issues +} + /// Filter out issues whose `issue_type` matches any ignored rule pattern. fn apply_ignore_rules(mut issues: Vec, ignore_rules: &[String]) -> Vec { if ignore_rules.is_empty() { @@ -389,6 +499,7 @@ fn is_valid_file_path(issue_file: &str, valid_files: &[String]) -> bool { #[cfg(test)] mod tests { use super::*; + use crate::engine::Severity; #[test] fn resolve_prompt_inline_takes_priority() { @@ -427,4 +538,161 @@ mod tests { "system_prompt_file outside project root should be rejected" ); } + + #[test] + fn secret_fp_filter_removes_struct_field_declarations() { + use crate::engine::diff_parser::*; + + // Simulate a diff with a struct field declaration (not a hardcoded secret) + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("crates/uteke-cli/src/cli.rs".to_string()), + language: "rs".to_string(), + chunks: vec![DiffHunk { + old_start: 230, + old_count: 0, + new_start: 234, + new_count: 2, + header: "".to_string(), + lines: vec![ + DiffLine { + line_type: DiffLineType::Add, + content: " extract_api_key: Option,".to_string(), + old_line_no: None, + new_line_no: Some(236), + }, + DiffLine { + line_type: DiffLineType::Add, + content: " extract_base_url: Option,".to_string(), + old_line_no: None, + new_line_no: Some(237), + }, + ], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ReviewIssue { + file: "crates/uteke-cli/src/cli.rs".to_string(), + line: Some(236), + severity: Severity::Critical, + issue_type: Some("security".to_string()), + title: "Hardcoded password or secret in variable".to_string(), + body: "Static security scanner detected...".to_string(), + suggested_fix: None, + }]; + + let result = apply_llm_secret_fp_filter(issues, &diff_chunks); + assert!( + result.is_empty(), + "struct field declaration should be filtered out" + ); + } + + #[test] + fn secret_fp_filter_keeps_actual_hardcoded_secrets() { + use crate::engine::diff_parser::*; + + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("src/config.rs".to_string()), + language: "rs".to_string(), + chunks: vec![DiffHunk { + old_start: 10, + old_count: 0, + new_start: 15, + new_count: 1, + header: "".to_string(), + lines: vec![DiffLine { + line_type: DiffLineType::Add, + content: " let api_key = \"sk-12345abcdef\";".to_string(), + old_line_no: None, + new_line_no: Some(15), + }], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ReviewIssue { + file: "src/config.rs".to_string(), + line: Some(15), + severity: Severity::Critical, + issue_type: Some("security".to_string()), + title: "Hardcoded password or secret in variable".to_string(), + body: "API key hardcoded...".to_string(), + suggested_fix: None, + }]; + + let result = apply_llm_secret_fp_filter(issues, &diff_chunks); + assert_eq!(result.len(), 1, "actual hardcoded secret should be kept"); + } + + #[test] + fn secret_fp_filter_keeps_non_security_findings() { + use crate::engine::diff_parser::*; + + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("src/main.rs".to_string()), + language: "rs".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 0, + new_start: 1, + new_count: 1, + header: "".to_string(), + lines: vec![DiffLine { + line_type: DiffLineType::Add, + content: " api_key: String,".to_string(), + old_line_no: None, + new_line_no: Some(1), + }], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ReviewIssue { + file: "src/main.rs".to_string(), + line: Some(1), + severity: Severity::Minor, + issue_type: Some("bugs".to_string()), + title: "Use of unwrap()".to_string(), + body: "This can panic".to_string(), + suggested_fix: None, + }]; + + let result = apply_llm_secret_fp_filter(issues, &diff_chunks); + assert_eq!(result.len(), 1, "non-security findings should pass through"); + } + + #[test] + fn secret_fp_filter_keeps_findings_with_unknown_lines() { + use crate::engine::diff_parser::*; + + // Empty diff — finding references a line not in the diff + let diff_chunks: Vec = vec![]; + + let issues = vec![ReviewIssue { + file: "src/config.rs".to_string(), + line: Some(999), + severity: Severity::Critical, + issue_type: Some("security".to_string()), + title: "Hardcoded password or secret in variable".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_llm_secret_fp_filter(issues, &diff_chunks); + assert_eq!( + result.len(), + 1, + "unknown lines should be kept (better safe than sorry)" + ); + } } diff --git a/src/git/diff.rs b/src/git/diff.rs index 4678ec0..fa6502d 100644 --- a/src/git/diff.rs +++ b/src/git/diff.rs @@ -156,7 +156,7 @@ pub fn get_repo_info() -> std::result::Result<(String, String, String), CoraErro let remote_url = repo .find_remote("origin") .ok() - .and_then(|r| r.url().map(std::string::ToString::to_string)) + .and_then(|r| r.url().ok().map(std::string::ToString::to_string)) .unwrap_or_default(); let (owner, repo_name) = parse_remote_url(&remote_url); From 7dff4a89c663af80c2f83df0305985a7b2ccb3d7 Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Sat, 27 Jun 2026 15:11:36 +0700 Subject: [PATCH 02/16] fix(security): tighten command injection regex + add ignore rules logging (#328) - Tighten injection/exec regex: only flag subprocess.run when dynamic input signals present (f-string, format(), shell=True, string concat with user input). Literal list calls like subprocess.run(cmd) are safe. - Add debug logging to apply_ignore_rules for better diagnostics - Add unit tests for ignore_rules (title match, issue_type match, empty, case insensitive) - Add unit tests for security_scanner (no FP on literal list, triggers on f-string and shell=True) Fixes #326 Co-authored-by: ajianaz --- src/engine/review.rs | 89 ++++++++++++++++++++++++++++++++++ src/engine/security_scanner.rs | 58 +++++++++++++++++++++- 2 files changed, 146 insertions(+), 1 deletion(-) diff --git a/src/engine/review.rs b/src/engine/review.rs index db0ae5f..185c3d3 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -478,6 +478,7 @@ fn apply_ignore_rules(mut issues: Vec, ignore_rules: &[String]) -> return issues; } + let before = issues.len(); issues.retain(|issue| { !ignore_rules.iter().any(|pattern| { let pattern_lower = pattern.to_lowercase(); @@ -486,6 +487,15 @@ fn apply_ignore_rules(mut issues: Vec, ignore_rules: &[String]) -> || issue.title.to_lowercase().contains(&pattern_lower) }) }); + let filtered = before - issues.len(); + if filtered > 0 { + debug!( + filtered, + remaining = issues.len(), + rules = ignore_rules.len(), + "filtered issues via ignore rules" + ); + } issues } @@ -695,4 +705,83 @@ mod tests { "unknown lines should be kept (better safe than sorry)" ); } + + #[test] + fn ignore_rules_filters_by_title_match() { + let issues = vec![ + ReviewIssue { + file: "cli.rs".to_string(), + line: Some(236), + severity: Severity::Critical, + issue_type: Some("rule".to_string()), + title: "Command injection via exec/system with dynamic input".to_string(), + body: "Static security scanner detected...".to_string(), + suggested_fix: None, + }, + ReviewIssue { + file: "main.rs".to_string(), + line: Some(10), + severity: Severity::Major, + issue_type: Some("security".to_string()), + title: "SQL injection via string concatenation".to_string(), + body: "...".to_string(), + suggested_fix: None, + }, + ]; + + let rules = vec!["Command injection via exec/system with dynamic input".to_string()]; + let result = apply_ignore_rules(issues, &rules); + assert_eq!(result.len(), 1); + assert_eq!(result[0].title, "SQL injection via string concatenation"); + } + + #[test] + fn ignore_rules_filters_by_issue_type_match() { + let issues = vec![ReviewIssue { + file: "test.py".to_string(), + line: Some(50), + severity: Severity::Minor, + issue_type: Some("style".to_string()), + title: "Some style issue".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let rules = vec!["style".to_string()]; + let result = apply_ignore_rules(issues, &rules); + assert!(result.is_empty()); + } + + #[test] + fn ignore_rules_empty_keeps_all() { + let issues = vec![ReviewIssue { + file: "f.rs".to_string(), + line: Some(1), + severity: Severity::Critical, + issue_type: Some("rule".to_string()), + title: "Any finding".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_ignore_rules(issues, &[]); + assert_eq!(result.len(), 1); + } + + #[test] + fn ignore_rules_case_insensitive() { + let issues = vec![ReviewIssue { + file: "f.rs".to_string(), + line: Some(1), + severity: Severity::Critical, + issue_type: Some("rule".to_string()), + title: "HARDCODED password or SECRET in variable".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let rules = vec!["Hardcoded Password Or Secret".to_string()]; + let result = apply_ignore_rules(issues, &rules); + assert!(result.is_empty()); + } } diff --git a/src/engine/security_scanner.rs b/src/engine/security_scanner.rs index 3bf5fa9..8f6ed95 100644 --- a/src/engine/security_scanner.rs +++ b/src/engine/security_scanner.rs @@ -63,7 +63,10 @@ pub static PATTERNS: &[SecurityPattern] = &[ SecurityPattern { id: "injection/exec", name: "Command injection via exec/system with dynamic input", - regex: r"(?i)(?:exec|system|popen|subprocess\.(?:call|run))\s*\(", + // Only flag when there's a dynamic input signal on the same line + // (f-string, format(), string concat with +, shell=True, or raw user input). + // subprocess.run(["cmd", "arg"]) with literal list is safe and should not trigger. + regex: r#"(?i)(?:exec|system|popen|subprocess\.(?:call|run))\s*\((?:[^)]*(?:f"|f'|format\(|\.format\(|shell\s*=\s*True|\+\s*(?:req|request|input|params|data|user|query)|%s|%\(.*\)))"#, severity: Severity::Critical, }, // ── Auth issues ── @@ -312,4 +315,57 @@ mod tests { // Critical (hardcoded-secret) should come before Minor (debug) assert!(findings[0].severity >= findings[findings.len() - 1].severity); } + + #[test] + fn subprocess_run_with_literal_list_no_false_positive() { + let chunks = vec![make_chunk( + "src/provider.py", + &["cmd = [self._bin, 'recall', query]", "subprocess.run(cmd)"], + )]; + let findings = scan_security(&chunks, 10); + let exec_findings: Vec<_> = findings + .iter() + .filter(|f| f.rule_id == "injection/exec") + .collect(); + assert!( + exec_findings.is_empty(), + "subprocess.run(cmd) with controlled list should not trigger" + ); + } + + #[test] + fn subprocess_run_with_fstring_triggers() { + let chunks = vec![make_chunk( + "src/handler.py", + &["subprocess.run(f'cat {user_input}')"], + )]; + let findings = scan_security(&chunks, 10); + let exec_findings: Vec<_> = findings + .iter() + .filter(|f| f.rule_id == "injection/exec") + .collect(); + assert_eq!( + exec_findings.len(), + 1, + "subprocess.run with f-string should trigger" + ); + } + + #[test] + fn subprocess_run_with_shell_true_triggers() { + let chunks = vec![make_chunk( + "src/handler.py", + &["subprocess.run(cmd, shell=True)"], + )]; + let findings = scan_security(&chunks, 10); + let exec_findings: Vec<_> = findings + .iter() + .filter(|f| f.rule_id == "injection/exec") + .collect(); + assert_eq!( + exec_findings.len(), + 1, + "subprocess.run with shell=True should trigger" + ); + } } From 4cad27eba862962880910ec12d6f1333d2954ed8 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 2 Jul 2026 08:54:48 +0700 Subject: [PATCH 03/16] fix: handle serde duplicate field error when providers send both legacy and new usage field names GPT-5.4 (and potentially other newer models) return both legacy (prompt_tokens/completion_tokens) and new (input_tokens/output_tokens) field names in the usage object simultaneously. serde_json >= 1.0.120 tracks visited fields during struct deserialization. When an alias maps to a field already set from the primary key, it raises 'duplicate field' error, breaking all reviews using affected models. Fix: Replace serde alias-based Usage deserialization with manual parse_usage_value() that extracts from serde_json::Value with explicit preference order (primary > camelCase > new). Updated both streaming and non-streaming code paths. Closes #330 --- src/engine/llm.rs | 170 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 151 insertions(+), 19 deletions(-) diff --git a/src/engine/llm.rs b/src/engine/llm.rs index f2ee77c..beaa8d2 100644 --- a/src/engine/llm.rs +++ b/src/engine/llm.rs @@ -66,10 +66,15 @@ struct ChatRequest { } /// Response from /chat/completions. +/// +/// `usage` is parsed as raw `serde_json::Value` to avoid serde's duplicate-field +/// detection when a provider sends both legacy (`prompt_tokens`) and new +/// (`input_tokens`) field names simultaneously (e.g. GPT-5.4). The value is +/// converted to a typed `Usage` via [`parse_usage_value`] in post-processing. #[derive(Debug, Clone, Deserialize)] struct ChatResponse { choices: Vec, - usage: Option, + usage: Option, } #[derive(Debug, Clone, Deserialize)] @@ -79,25 +84,61 @@ struct ChatChoice { /// Usage statistics from the LLM API response. /// -/// OpenAI-compatible providers return these in every non-streaming response, -/// and in the final SSE chunk when `stream_options.include_usage` is set. -/// Some providers send them under snake_case (`prompt_tokens`), others under -/// camelCase (`promptTokens`) — both are accepted here. -/// -/// `total_tokens` is accepted but not used by cora (we sum prompt+completion -/// instead) — it is kept for completeness so that providers that omit -/// `prompt_tokens` / `completion_tokens` in favour of a single `total_tokens` -/// can still be surfaced downstream. -#[derive(Debug, Clone, Deserialize)] +/// Constructed via [`parse_usage_value`] which accepts a raw `serde_json::Value` +/// and handles providers that send legacy field names (`prompt_tokens`, +/// `completion_tokens`), new field names (`input_tokens`, `output_tokens`), +/// or both simultaneously (e.g. GPT-5.4). +#[derive(Debug, Clone, Default)] pub(crate) struct Usage { - #[serde(default, alias = "promptTokens", alias = "input_tokens")] prompt_tokens: u32, - #[serde(default, alias = "completionTokens", alias = "output_tokens")] completion_tokens: u32, - #[serde(default, alias = "totalTokens")] total_tokens: u32, } +/// Extract a typed [`Usage`] from a raw `serde_json::Value`. +/// +/// Handles three naming conventions that OpenAI-compatible providers use: +/// +/// | Field | Legacy (OpenAI) | New (GPT-5+) | CamelCase (Azure) | +/// |----------------|-------------------|--------------------|--------------------| +/// | input | `prompt_tokens` | `input_tokens` | `promptTokens` | +/// | output | `completion_tokens` | `output_tokens` | `completionTokens` | +/// | total | `total_tokens` | `total_tokens` | `totalTokens` | +/// +/// Some providers (notably GPT-5.4) send **both** legacy and new names for the +/// same value. Direct serde deserialization with aliases would hit serde_json's +/// duplicate-field guard (>= 1.0.120), so we extract manually via `Value` +/// and pick the first non-zero value in preference order. +fn parse_usage_value(val: &Value) -> Option { + let obj = val.as_object()?; + + let prompt_tokens = obj + .get("prompt_tokens") + .and_then(|v| v.as_u64()) + .or_else(|| obj.get("promptTokens").and_then(|v| v.as_u64())) + .or_else(|| obj.get("input_tokens").and_then(|v| v.as_u64())) + .unwrap_or(0) as u32; + + let completion_tokens = obj + .get("completion_tokens") + .and_then(|v| v.as_u64()) + .or_else(|| obj.get("completionTokens").and_then(|v| v.as_u64())) + .or_else(|| obj.get("output_tokens").and_then(|v| v.as_u64())) + .unwrap_or(0) as u32; + + let total_tokens = obj + .get("total_tokens") + .and_then(|v| v.as_u64()) + .or_else(|| obj.get("totalTokens").and_then(|v| v.as_u64())) + .unwrap_or(0) as u32; + + Some(Usage { + prompt_tokens, + completion_tokens, + total_tokens, + }) +} + impl Usage { /// Effective input tokens. /// @@ -300,10 +341,12 @@ async fn chat_completion( .map(|c| c.message.content.clone()) .unwrap_or_default(); - debug!(tokens = ?parsed.usage, "LLM response received"); - tracing::Span::current().record("tokens_used", parsed.usage.as_ref().map(|u| u.total_tokens)); + let usage = parsed.usage.as_ref().and_then(parse_usage_value); - Ok((content, parsed.usage)) + debug!(tokens = ?usage, "LLM response received"); + tracing::Span::current().record("tokens_used", usage.as_ref().map(|u| u.total_tokens)); + + Ok((content, usage)) } /// Create an animated spinner for LLM operations. @@ -631,17 +674,20 @@ fn extract_stream_content(parsed: &Value) -> Option<&str> { /// The `usage` field appears either at top level (OpenAI convention, sent in /// the final chunk when `stream_options.include_usage` is set) or inside the /// final choice's delta (some Azure / third-party providers). +/// +/// Uses [`parse_usage_value`] to avoid serde's duplicate-field guard when a +/// provider sends both legacy and new field names simultaneously. fn extract_stream_usage(parsed: &Value) -> Option { parsed .get("usage") - .and_then(|u| serde_json::from_value::(u.clone()).ok()) + .and_then(parse_usage_value) .or_else(|| { parsed .get("choices") .and_then(|c| c.get(0)) .and_then(|c| c.get("delta")) .and_then(|d| d.get("usage")) - .and_then(|u| serde_json::from_value::(u.clone()).ok()) + .and_then(parse_usage_value) }) } @@ -1420,6 +1466,92 @@ mod tests { assert_eq!(token_usage.output_tokens, 150); // total - prompt } + // ─── parse_usage_value (GPT-5.4 dual-field handling) ─── + + #[test] + fn parse_usage_value_legacy_fields() { + // Traditional OpenAI format: prompt_tokens / completion_tokens + let val = serde_json::json!({ + "prompt_tokens": 2615, + "completion_tokens": 581, + "total_tokens": 3196 + }); + let usage = parse_usage_value(&val).unwrap(); + assert_eq!(usage.prompt_tokens, 2615); + assert_eq!(usage.completion_tokens, 581); + assert_eq!(usage.total_tokens, 3196); + } + + #[test] + fn parse_usage_value_new_fields_only() { + // Some providers only send input_tokens / output_tokens + let val = serde_json::json!({ + "input_tokens": 1000, + "output_tokens": 200, + "total_tokens": 1200 + }); + let usage = parse_usage_value(&val).unwrap(); + assert_eq!(usage.prompt_tokens, 1000); + assert_eq!(usage.completion_tokens, 200); + assert_eq!(usage.total_tokens, 1200); + } + + #[test] + fn parse_usage_value_gpt54_dual_fields() { + // GPT-5.4 sends BOTH legacy and new field names — this is the + // scenario that previously caused serde duplicate-field error. + let val = serde_json::json!({ + "prompt_tokens": 2615, + "completion_tokens": 581, + "total_tokens": 3196, + "prompt_tokens_details": {"cached_tokens": 0}, + "completion_tokens_details": {"reasoning_tokens": 0}, + "input_tokens": 2615, + "output_tokens": 581, + "input_tokens_details": null + }); + let usage = parse_usage_value(&val).unwrap(); + // Must prefer primary (prompt_tokens) over alias (input_tokens) + assert_eq!(usage.prompt_tokens, 2615); + assert_eq!(usage.completion_tokens, 581); + assert_eq!(usage.total_tokens, 3196); + } + + #[test] + fn parse_usage_value_camelcase_fields() { + // Azure / some third-party providers use camelCase + let val = serde_json::json!({ + "promptTokens": 500, + "completionTokens": 100, + "totalTokens": 600 + }); + let usage = parse_usage_value(&val).unwrap(); + assert_eq!(usage.prompt_tokens, 500); + assert_eq!(usage.completion_tokens, 100); + assert_eq!(usage.total_tokens, 600); + } + + #[test] + fn parse_usage_value_missing_fields_defaults_to_zero() { + // Partial usage (e.g. streaming final chunk) + let val = serde_json::json!({ + "prompt_tokens": 100 + }); + let usage = parse_usage_value(&val).unwrap(); + assert_eq!(usage.prompt_tokens, 100); + assert_eq!(usage.completion_tokens, 0); + assert_eq!(usage.total_tokens, 0); + } + + #[test] + fn parse_usage_value_non_object_returns_none() { + let val = serde_json::json!("not an object"); + assert!(parse_usage_value(&val).is_none()); + + let val = serde_json::json!(42); + assert!(parse_usage_value(&val).is_none()); + } + // ─── Various issue_type values ─── #[test] From aad16d50daf75ab8f32e7d416eab92ffcdd3b243 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 2 Jul 2026 09:07:01 +0700 Subject: [PATCH 04/16] style: fix cargo fmt --- src/engine/llm.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/engine/llm.rs b/src/engine/llm.rs index beaa8d2..8325054 100644 --- a/src/engine/llm.rs +++ b/src/engine/llm.rs @@ -678,17 +678,14 @@ fn extract_stream_content(parsed: &Value) -> Option<&str> { /// Uses [`parse_usage_value`] to avoid serde's duplicate-field guard when a /// provider sends both legacy and new field names simultaneously. fn extract_stream_usage(parsed: &Value) -> Option { - parsed - .get("usage") - .and_then(parse_usage_value) - .or_else(|| { - parsed - .get("choices") - .and_then(|c| c.get(0)) - .and_then(|c| c.get("delta")) - .and_then(|d| d.get("usage")) - .and_then(parse_usage_value) - }) + parsed.get("usage").and_then(parse_usage_value).or_else(|| { + parsed + .get("choices") + .and_then(|c| c.get(0)) + .and_then(|c| c.get("delta")) + .and_then(|d| d.get("usage")) + .and_then(parse_usage_value) + }) } /// Scan a batch of file contents using the LLM. Returns issues found. From 53e2833f883239f4fce034b665fddc0f7bf9b10a Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 2 Jul 2026 11:38:21 +0700 Subject: [PATCH 05/16] fix: resolve 4 critical security vulnerabilities (P0) - Fix #32: Command injection via in commit_cmd.rs Replace sh -c shell execution with direct Command::new(program).args(args) to prevent shell injection through EDITOR env variable. - Fix #61: Path traversal in JS/TS import resolver Add safe_join() helper that canonicalizes paths and verifies containment within project root. Apply to JS/TS import resolution and all other project_root.join() calls involving user-controlled paths. - Fix #62: Path traversal in symbol resolver Use safe_join() in resolve_symbols, test file resolution, and read_entry_content to prevent reading files outside project root. - Fix #91: API key file written before chmod (TOCTOU race) Create file with 0o600 permissions from the start using OpenOptionsExt on Unix, eliminating the window between write and chmod. Closes #332 --- src/commands/commit_cmd.rs | 17 +++++---- src/config/loader.rs | 31 +++++++++++----- src/engine/context/resolver.rs | 68 +++++++++++++++++++++++++++++++--- 3 files changed, 94 insertions(+), 22 deletions(-) diff --git a/src/commands/commit_cmd.rs b/src/commands/commit_cmd.rs index a321d92..b523011 100644 --- a/src/commands/commit_cmd.rs +++ b/src/commands/commit_cmd.rs @@ -372,13 +372,16 @@ fn open_editor(initial: &str) -> Result { std::fs::write(&tmp_path, initial) .with_context(|| format!("Failed to write temp file: {}", tmp_path.display()))?; - let path_str = tmp_path.to_string_lossy().to_string(); - - // Use sh -c to handle multi-word editors properly - let shell_cmd = format!("{editor} '{path_str}'"); - let status = std::process::Command::new("sh") - .arg("-c") - .arg(&shell_cmd) + // Split $EDITOR into program + args to avoid shell injection via `sh -c`. + // Handles editors with flags like `code --wait` or `vim -p`. + let editor_parts: Vec<&str> = editor.split_whitespace().collect(); + let (program, args) = match editor_parts.split_first() { + Some((p, a)) => (p, a), + None => anyhow::bail!("$EDITOR is empty"), + }; + let status = std::process::Command::new(program) + .args(args) + .arg(&tmp_path) .status() .with_context(|| format!("Failed to open editor: {editor}"))?; diff --git a/src/config/loader.rs b/src/config/loader.rs index 90fc40a..49bf9a7 100644 --- a/src/config/loader.rs +++ b/src/config/loader.rs @@ -435,19 +435,32 @@ pub fn save_api_key(key: &str) -> std::result::Result<(), CoraError> { table.insert("api_key".to_string(), toml::Value::String(key.to_string())); let content = table.to_string(); - std::fs::write(&path, content) - .map_err(|e| CoraError::AuthError(format!("{}: {}", path.display(), e)))?; - - debug!(path = %path.display(), "saved API key"); - - // Restrict permissions to owner only (0o600) + // Create the file with restrictive permissions from the start (0o600), + // avoiding the TOCTOU window between write and chmod. #[cfg(unix)] { - use std::os::unix::fs::PermissionsExt; - let perms = std::fs::Permissions::from_mode(0o600); - std::fs::set_permissions(&path, perms)?; + use std::os::unix::fs::OpenOptionsExt; + let mut file = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(&path) + .map_err(|e| CoraError::AuthError(format!("{}: {}", path.display(), e)))?; + use std::io::Write; + file.write_all(content.as_bytes()) + .map_err(|e| CoraError::AuthError(format!("{}: {}", path.display(), e)))?; + } + #[cfg(not(unix))] + { + // Non-Unix platforms: write then attempt to restrict permissions. + // Best-effort — platform support for file permissions varies. + std::fs::write(&path, content) + .map_err(|e| CoraError::AuthError(format!("{}: {}", path.display(), e)))?; } + debug!(path = %path.display(), "saved API key"); + Ok(()) } diff --git a/src/engine/context/resolver.rs b/src/engine/context/resolver.rs index 46521a3..9635469 100644 --- a/src/engine/context/resolver.rs +++ b/src/engine/context/resolver.rs @@ -11,7 +11,7 @@ //! Additionally, test file mapping is supported via naming conventions. use std::collections::HashMap; -use std::path::Path; +use std::path::{Path, PathBuf}; use tracing::debug; @@ -27,6 +27,34 @@ const MAX_TYPE_LINES: usize = 50; /// Maximum lines to read per test function. const MAX_TEST_LINES: usize = 30; +/// Safely join a relative path with a project root, verifying that the +/// resulting path stays within the project root (prevents path traversal). +/// Returns `None` if the resolved path escapes the project root or +/// canonicalization fails for an existing file. +fn safe_join(project_root: &Path, relative: &str) -> Option { + let joined = project_root.join(relative); + // Canonicalize to resolve any `..` or symlinks. + // If the file doesn't exist yet, canonicalize just the project_root + // and check that the joined path starts with it as a prefix. + let canonical_root = std::fs::canonicalize(project_root).ok()?; + if joined.exists() { + let canonical_joined = std::fs::canonicalize(&joined).ok()?; + if canonical_joined.starts_with(&canonical_root) { + Some(canonical_joined) + } else { + None + } + } else { + // File doesn't exist yet; verify the joined path doesn't escape + // the project root by canonicalizing what we can and checking prefixes. + if joined.starts_with(&project_root) { + Some(joined) + } else { + None + } + } +} + /// Build the full context chain from extracted symbols. /// /// This is the main entry point: extract → resolve → read → budget → assemble. @@ -137,8 +165,14 @@ fn resolve_symbols( continue; } - // Check if the entry's file exists - let full_path = project_root.join(&entry.file); + // Check if the entry's file exists and stays within project root + let full_path = match safe_join(project_root, &entry.file) { + Some(p) => p, + None => { + debug!(file = %entry.file, "path traversal detected, skipping"); + continue; + } + }; if !full_path.exists() { debug!(file = %entry.file, "resolved file does not exist, skipping"); continue; @@ -248,7 +282,13 @@ fn resolve_import( for ext in &extensions { let candidate = format!("{}.{}", resolved.display(), ext); - let full = project_root.join(&candidate); + let full = match safe_join(project_root, &candidate) { + Some(p) => p, + None => { + debug!(file = %candidate, "path traversal detected in JS/TS import, skipping"); + continue; + } + }; if full.exists() { let line_end = find_definition_end(&full); entries.push(ContextEntry { @@ -624,7 +664,13 @@ fn add_test_mappings( let candidates = test_file_candidates(&sym.file); for candidate in candidates { - let full = project_root.join(&candidate); + let full = match safe_join(project_root, &candidate) { + Some(p) => p, + None => { + debug!(file = %candidate, "path traversal detected in test resolution, skipping"); + continue; + } + }; if full.exists() { let line_end = find_definition_end(&full); entries.push(ContextEntry { @@ -697,7 +743,17 @@ fn test_file_candidates(source: &str) -> Vec { /// Read the content for a context entry, respecting line range and caps. fn read_entry_content(entry: &ContextEntry, project_root: &Path) -> String { - let full_path = project_root.join(&entry.file); + let full_path = match safe_join(project_root, &entry.file) { + Some(p) if p.exists() => p, + Some(_) => { + debug!(file = %entry.file, "context entry file does not exist"); + return String::new(); + } + None => { + debug!(file = %entry.file, "path traversal detected in read_entry_content"); + return String::new(); + } + }; let content = match std::fs::read_to_string(&full_path) { Ok(c) => c, Err(e) => { From ce564449afb8c7897e6e34c855f1adf3837e84fb Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 2 Jul 2026 11:44:07 +0700 Subject: [PATCH 06/16] fix: resolve clippy needless_borrows_for_generic_args warning --- src/engine/context/resolver.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/context/resolver.rs b/src/engine/context/resolver.rs index 9635469..79df8bd 100644 --- a/src/engine/context/resolver.rs +++ b/src/engine/context/resolver.rs @@ -47,7 +47,7 @@ fn safe_join(project_root: &Path, relative: &str) -> Option { } else { // File doesn't exist yet; verify the joined path doesn't escape // the project root by canonicalizing what we can and checking prefixes. - if joined.starts_with(&project_root) { + if joined.starts_with(project_root) { Some(joined) } else { None From 1bd7ffb4094a031de5ad04257486d6e1503d9172 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 2 Jul 2026 11:54:53 +0700 Subject: [PATCH 07/16] fix: resolve silent data corruption bugs (P1) Fixes: - #42 Severity sort inverted: sort descending (Critical first) so max_findings truncation drops least important findings - #45 Security findings dropped on LLM failure: include security_findings in fallback condition and summary - #52 HashMap nondeterministic hash: sort HashMap entries by key before hashing for deterministic filenames - #53 Debt score trend wrong: compute recent_avg_quality from recent slice instead of overall average - #54 Category trend change inflated: build recent_category_counts from recent slice for accurate delta - #92 Config precedence inverted: env vars > config > auto-detect > defaults - #93 context_chain config ignored: copy context_chain from ReviewSection in merge_into - #96 Hook install overwrites existing hooks: use sentinel marker, backup non-cora hooks, compose wrapper Refs #333 --- src/config/loader.rs | 30 +++++++++++++++++++++----- src/config/schema.rs | 3 +++ src/engine/debt_tracker.rs | 44 +++++++++++++++++++++++++++++--------- src/engine/review.rs | 9 ++++++-- src/engine/rules/mod.rs | 20 +++++++++++++---- src/hook/install.rs | 37 ++++++++++++++++++++++++-------- 6 files changed, 113 insertions(+), 30 deletions(-) diff --git a/src/config/loader.rs b/src/config/loader.rs index 49bf9a7..d402128 100644 --- a/src/config/loader.rs +++ b/src/config/loader.rs @@ -339,19 +339,39 @@ pub fn build_llm_config( let cora_base_url = std::env::var("CORA_BASE_URL").ok(); let provider = cora_provider - .or_else(|| auto_preset.map(|p| p.name.to_string())) + .or_else(|| { + if config.provider.provider + != crate::config::schema::Config::default().provider.provider + { + Some(config.provider.provider.clone()) + } else { + auto_preset.map(|p| p.name.to_string()) + } + }) .unwrap_or_else(|| config.provider.provider.clone()); let model = cora_model - .or_else(|| auto_preset.map(|p| p.default_model.to_string())) + .or_else(|| { + if config.provider.model != crate::config::schema::Config::default().provider.model { + Some(config.provider.model.clone()) + } else { + auto_preset.map(|p| p.default_model.to_string()) + } + }) .unwrap_or_else(|| config.provider.model.clone()); let base_url = cora_base_url .or_else(|| { - // Check if the auto-detected preset has a custom URL override - auto_preset.and_then(|p| std::env::var(p.env_url).ok()) + if config.provider.base_url + != crate::config::schema::Config::default().provider.base_url + { + Some(config.provider.base_url.clone()) + } else { + auto_preset + .and_then(|p| std::env::var(p.env_url).ok()) + .or_else(|| auto_preset.map(|p| p.default_base_url.to_string())) + } }) - .or_else(|| auto_preset.map(|p| p.default_base_url.to_string())) .unwrap_or_else(|| config.provider.base_url.clone()); Ok(LLMConfig { diff --git a/src/config/schema.rs b/src/config/schema.rs index 705a0f2..5fdb9db 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -424,6 +424,9 @@ impl CoraFile { if let Some(sa) = &r.static_analysis { config.static_analysis.clone_from(sa); } + if let Some(cc) = &r.context_chain { + config.context_chain.clone_from(cc); + } } if let Some(s) = &self.scan { if let Some(v) = &s.system_prompt { diff --git a/src/engine/debt_tracker.rs b/src/engine/debt_tracker.rs index b8a56dd..3cf25e9 100644 --- a/src/engine/debt_tracker.rs +++ b/src/engine/debt_tracker.rs @@ -211,13 +211,19 @@ fn snapshot_filename(snapshot: &DebtSnapshot) -> String { use std::hash::{Hash, Hasher}; let mut hasher = std::collections::hash_map::DefaultHasher::new(); snapshot.timestamp.hash(&mut hasher); - for (k, v) in &snapshot.findings { - k.hash(&mut hasher); - v.hash(&mut hasher); - } - for (k, v) in &snapshot.categories { - k.hash(&mut hasher); - v.hash(&mut hasher); + { + let mut findings_sorted: Vec<_> = snapshot.findings.iter().collect(); + findings_sorted.sort_by_key(|(k, _)| *k); + for (k, v) in findings_sorted { + k.hash(&mut hasher); + v.hash(&mut hasher); + } + let mut categories_sorted: Vec<_> = snapshot.categories.iter().collect(); + categories_sorted.sort_by_key(|(k, _)| *k); + for (k, v) in categories_sorted { + k.hash(&mut hasher); + v.hash(&mut hasher); + } } let content_hash = format!("{:08x}", hasher.finish()); format!("{date}_{short_hash}_{content_hash}.json") @@ -418,7 +424,12 @@ pub fn aggregate(snapshots: &[DebtSnapshot]) -> DebtReport { } else { previous.iter().map(|s| s.quality_score).sum::() / previous.len() as f64 }; - let quality_score_change = quality_score_avg - previous_avg_quality; + let recent_avg_quality = if recent.is_empty() { + quality_score_avg + } else { + recent.iter().map(|s| s.quality_score).sum::() / recent.len() as f64 + }; + let quality_score_change = recent_avg_quality - previous_avg_quality; // Per-category with trend let mut category_reports = Vec::new(); @@ -429,12 +440,25 @@ pub fn aggregate(snapshots: &[DebtSnapshot]) -> DebtReport { } } + // Build recent category counts separately for accurate delta + let mut recent_categories: HashMap = HashMap::new(); + for snap in recent { + for (cat, count) in &snap.categories { + *recent_categories.entry(cat.clone()).or_insert(0) += count; + } + } + // Collect all unique category names - let mut all_cat_names: Vec = all_categories.keys().cloned().collect(); + let all_cat_names: std::collections::HashSet<&String> = all_categories + .keys() + .chain(recent_categories.keys()) + .chain(previous_categories.keys()) + .collect(); + let mut all_cat_names: Vec = all_cat_names.into_iter().cloned().collect(); all_cat_names.sort(); for cat_name in &all_cat_names { - let count = all_categories.get(cat_name).copied().unwrap_or(0); + let count = recent_categories.get(cat_name).copied().unwrap_or(0); let prev_count = previous_categories.get(cat_name).copied().unwrap_or(0); let change = count as i64 - prev_count as i64; diff --git a/src/engine/review.rs b/src/engine/review.rs index 185c3d3..623bd6d 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -264,13 +264,18 @@ async fn review_diff_inner( Ok(resp) => resp, Err(e) => { // LLM failed — return deterministic findings only (don't silently swallow them) - if !rule_findings.is_empty() || !secrets_findings.is_empty() { + if !rule_findings.is_empty() + || !secrets_findings.is_empty() + || !security_findings.is_empty() + { let n_rules = rule_findings.len(); let n_secrets = secrets_findings.len(); + let n_security = security_findings.len(); debug!( error = %e, rule_findings = n_rules, secrets_findings = n_secrets, + security_findings = n_security, "LLM call failed, returning deterministic findings only" ); let mut all_deterministic = @@ -282,7 +287,7 @@ async fn review_diff_inner( let mut fallback = ReviewResponse { issues: all_deterministic, summary: format!( - "LLM review failed: {e}. Showing {n_rules} rule + {n_secrets} secrets findings." + "LLM review failed: {e}. Showing {n_rules} rule + {n_secrets} secrets + {n_security} security findings." ), tokens_used: None, should_block: false, diff --git a/src/engine/rules/mod.rs b/src/engine/rules/mod.rs index 7b4859e..c8028f8 100644 --- a/src/engine/rules/mod.rs +++ b/src/engine/rules/mod.rs @@ -8,7 +8,17 @@ use tracing::debug; use crate::engine::diff_parser::{DiffLineType, FileChunk, parse_diff}; use crate::engine::rules::types::{RuleFinding, RulesConfig}; -use crate::engine::types::ReviewIssue; +use crate::engine::types::{ReviewIssue, Severity}; + +/// Map severity to a numeric rank for sorting (Critical=4, Major=3, Minor=2, Info=1). +fn severity_rank(sev: Severity) -> u8 { + match sev { + Severity::Critical => 4, + Severity::Major => 3, + Severity::Minor => 2, + Severity::Info => 1, + } +} /// Run all rules (built-in + custom) against parsed diff chunks. /// @@ -79,10 +89,12 @@ pub fn run_rules(chunks: &[FileChunk], config: &RulesConfig) -> Vec } } - // Sort by severity (Critical first) then by file + line + // Sort by severity descending (Critical first) then by file + line findings.sort_by(|a, b| { - a.severity - .cmp(&b.severity) + let rank_a = severity_rank(a.severity); + let rank_b = severity_rank(b.severity); + rank_b + .cmp(&rank_a) .then_with(|| a.file.cmp(&b.file)) .then_with(|| a.line.cmp(&b.line)) }); diff --git a/src/hook/install.rs b/src/hook/install.rs index a62e3f9..5a571ec 100644 --- a/src/hook/install.rs +++ b/src/hook/install.rs @@ -3,24 +3,43 @@ use tracing::debug; use crate::hook::template::HOOK_TEMPLATE; +/// Sentinel marker identifying a cora-managed hook. +const CORA_HOOK_SENTINEL: &str = "# cora-managed-hook"; + /// Install the pre-commit hook to `.git/hooks/pre-commit`. pub fn install_hook() -> Result { let hooks_dir = find_git_hooks_dir()?; let hook_path = hooks_dir.join("pre-commit"); - // Check if a hook already exists + // Check if a hook already exists and handle accordingly if hook_path.is_file() { let existing = std::fs::read_to_string(&hook_path)?; - if existing.contains("cora") { - // Already has cora hook — back up and overwrite - let backup = hooks_dir.join("pre-commit.cora.bak"); - std::fs::copy(&hook_path, &backup)?; - debug!(path = %backup.display(), "backed up existing hook"); + if existing.contains(CORA_HOOK_SENTINEL) { + // Already a cora-managed hook — just overwrite + debug!("existing hook is cora-managed, overwriting"); } else { - // Different hook — back up and append cora - let backup = hooks_dir.join("pre-commit.pre-cora.bak"); + // Non-cora hook — back it up and compose a wrapper + let backup = hooks_dir.join("pre-commit.bak"); std::fs::copy(&hook_path, &backup)?; - debug!(path = %backup.display(), "backed up existing hook"); + debug!(path = %backup.display(), "backed up existing non-cora hook"); + + // Build a wrapper that runs the original hook first, then cora + let wrapper = format!( + "{existing}\n\n# cora-managed-hook — the section below was added by `cora hook install`\n{HOOK_TEMPLATE}" + ); + std::fs::write(&hook_path, &wrapper) + .with_context(|| format!("failed to write {}", hook_path.display()))?; + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let perms = std::fs::Permissions::from_mode(0o755); + std::fs::set_permissions(&hook_path, perms)?; + } + + let path_str = hook_path.display().to_string(); + debug!(path = %path_str, "installed pre-commit hook (wrapped existing)"); + return Ok(path_str); } } From 8d69243397d56f66519f6193bc1d33cba998c893 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Tue, 7 Jul 2026 08:33:19 +0700 Subject: [PATCH 08/16] =?UTF-8?q?fix:=20update=20project-sync.yml=20?= =?UTF-8?q?=E2=80=94=20add=20PR=20trigger,=20continue-on-error,=20robust?= =?UTF-8?q?=20helpers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/workflows/project-sync.yml | 293 +++++++++++++++++------------ 1 file changed, 170 insertions(+), 123 deletions(-) diff --git a/.github/workflows/project-sync.yml b/.github/workflows/project-sync.yml index 907b34b..c081110 100644 --- a/.github/workflows/project-sync.yml +++ b/.github/workflows/project-sync.yml @@ -1,8 +1,10 @@ -name: Sync to CodeCoraDev Roadmap +name: Sync to CodeCorraDev Roadmap on: issues: types: [opened, reopened, closed, labeled] + pull_request: + types: [closed] # Manual trigger for backfill workflow_dispatch: @@ -10,59 +12,95 @@ permissions: issues: read repository-projects: read -env: - PROJECT_NODE_ID: ${{ secrets.PROJECT_NODE_ID }} - jobs: sync: runs-on: ubuntu-latest + # Non-critical: project board sync should not block or create noise + continue-on-error: true steps: - - name: Sync issue to Project Board + - name: Sync issue/PR to Project Board env: GH_TOKEN: ${{ secrets.PROJECT_BOARD_TOKEN }} + PROJECT_NODE_ID: ${{ secrets.PROJECT_NODE_ID }} + STATUS_FIELD_ID: PVTSSF_lADOEUV_vs4BahYpzhVYcGc + DONE_OPTION_ID: 98236657 run: | + set -e + EVENT="${{ github.event.action }}" - ISSUE_NUMBER="${{ github.event.issue.number }}" - ISSUE_NODE_ID="${{ github.event.issue.node_id }}" - ISSUE_STATE="${{ github.event.issue.state }}" + EVENT_NAME="${{ github.event_name }}" + ISSUE_NUMBER="${{ github.event.issue.number || '' }}" + ISSUE_NODE_ID="${{ github.event.issue.node_id || '' }}" + ISSUE_STATE="${{ github.event.issue.state || '' }}" + PR_NUMBER="${{ github.event.pull_request.number || '' }}" + PR_NODE_ID="${{ github.event.pull_request.node_id || '' }}" + PR_MERGED="${{ github.event.pull_request.merged || '' }}" + PR_URL="${{ github.event.pull_request.html_url || '' }}" REPO="${{ github.repository }}" echo "::group::Event Details" - echo "Event: $EVENT" - echo "Issue: #$ISSUE_NUMBER" - echo "State: $ISSUE_STATE" - echo "Repo: $REPO" - echo "Project: $PROJECT_NODE_ID" + echo "Event name: $EVENT_NAME" + echo "Action: $EVENT" + echo "Issue: #$ISSUE_NUMBER (state=$ISSUE_STATE)" + echo "PR: #$PR_NUMBER (merged=$PR_MERGED)" + echo "Repo: $REPO" + echo "Project: $PROJECT_NODE_ID" echo "::endgroup::" + # Skip when project secrets are not configured. if [ -z "$PROJECT_NODE_ID" ]; then echo "::warning::PROJECT_NODE_ID not set. Skipping." exit 0 fi - # ─── Helpers ─── - + # ------------------------------------------------------------------ + # Helper: add a content node (issue/PR) to the project if absent. + # Echoes the project item id (existing or newly created) to stdout. + # ------------------------------------------------------------------ add_to_project() { local node_id="$1" - RESULT=$(gh api graphql \ + local err_file + err_file=$(mktemp) + trap "rm -f '$err_file'" RETURN + + local stdout_result exit_code + set +e + stdout_result=$(gh api graphql \ -f query=" mutation { addProjectV2ItemById(input: {projectId: \"$PROJECT_NODE_ID\", contentId: \"$node_id\"}) { item { id } } - }" 2>&1) - if echo "$RESULT" | grep -q '"item"'; then - # Extract item ID - ITEM_ID=$(echo "$RESULT" | python3 -c "import sys,json; print(json.load(sys.stdin)['data']['addProjectV2ItemById']['item']['id'])" 2>/dev/null || echo "") - echo "✅ Added to project (item: $ITEM_ID)" - else - echo "⚠️ Add failed (may already exist): $RESULT" + }" 2>"$err_file") + exit_code=$? + set -e + local stderr_result + stderr_result=$(cat "$err_file") + + if [ $exit_code -ne 0 ]; then + echo "❌ gh api failed (exit $exit_code): $stderr_result" >&2 + return 1 fi + if echo "$stdout_result" | grep -q '"errors"'; then + echo "❌ GraphQL error: $stdout_result $stderr_result" >&2 + return 1 + fi + + local item_id + item_id=$(echo "$stdout_result" | python3 -c "import sys,json; print(json.load(sys.stdin)['data']['addProjectV2ItemById']['item']['id'])" 2>/dev/null || true) + if [ -n "$item_id" ]; then + echo "$item_id" + return 0 + fi + echo "⚠️ Unexpected response (may already exist): $stdout_result $stderr_result" >&2 + return 1 } - # Find the project item ID for this issue - find_item_id() { - local issue_node_id="$1" + # ------------------------------------------------------------------ + # Helper: look up an existing project item id for a content node. + # ------------------------------------------------------------------ + get_item_id() { + local content_node_id="$1" gh api graphql \ -f query=" query { @@ -73,133 +111,142 @@ jobs: id content { ... on Issue { id } + ... on PullRequest { id } } } } } } - }" 2>&1 | python3 -c " - import sys, json - try: - data = json.load(sys.stdin) - items = data['data']['node']['items']['nodes'] - for item in items: - c = item.get('content') - if c and c.get('id') == '$issue_node_id': - print(item['id']) - break - except Exception: - pass - " 2>/dev/null || echo "" + }" 2>/dev/null \ + | python3 -c " + import sys, json + data = json.load(sys.stdin) + target = '$content_node_id' + for n in data['data']['node']['items']['nodes']: + c = n.get('content') or {} + if c.get('id') == target: + print(n['id']) + break + " 2>/dev/null } - # Resolve Status field ID + option ID by name - # Args: $1 = option name (e.g. "Todo", "Done", "In Progress") - set_status() { - local target_status="$1" - local item_id="$2" - - if [ -z "$item_id" ]; then - echo "⚠️ No item ID — cannot set status" - return 1 - fi - - # Query field + option IDs dynamically - FIELD_DATA=$(gh api graphql \ - -f query=" - query { - node(id: \"$PROJECT_NODE_ID\") { - ... on ProjectV2 { - fields(first: 20) { - nodes { - ... on ProjectV2SingleSelectField { - id - name - options { id name } - } - } - } - } - } - }" 2>&1) - - # Extract Status field ID and option ID - FIELD_INFO=$(echo "$FIELD_DATA" | python3 -c " - import sys, json - try: - data = json.load(sys.stdin) - fields = data['data']['node']['fields']['nodes'] - for f in fields: - if f.get('name') == 'Status': - print(f['id']) - for opt in f.get('options', []): - if opt['name'] == '$target_status': - print(opt['id']) - break - break - except Exception: - pass - " 2>/dev/null || echo "") - - FIELD_ID=$(echo "$FIELD_INFO" | head -1) - OPTION_ID=$(echo "$FIELD_INFO" | tail -1) - - if [ -z "$FIELD_ID" ] || [ -z "$OPTION_ID" ]; then - echo "⚠️ Could not resolve Status field or option '$target_status'" - return 1 - fi - - # Update the field - UPDATE_RESULT=$(gh api graphql \ + # ------------------------------------------------------------------ + # Helper: set Status field to Done for a project item. + # ------------------------------------------------------------------ + set_done() { + local item_id="$1" + echo "Setting item $item_id → Done" + gh api graphql \ -f query=" mutation { updateProjectV2ItemFieldValue(input: { - projectId: \"$PROJECT_NODE_ID\" - itemId: \"$item_id\" - fieldId: \"$FIELD_ID\" - value: { singleSelectOptionId: \"$OPTION_ID\" } + projectId: \"$PROJECT_NODE_ID\", + itemId: \"$item_id\", + fieldId: \"$STATUS_FIELD_ID\", + value: { singleSelectOptionId: \"$DONE_OPTION_ID\" } }) { projectV2Item { id } } - }" 2>&1) + }" 2>&1 + } - if echo "$UPDATE_RESULT" | grep -q 'projectV2Item'; then - echo "✅ Status set to '$target_status'" + # ================================================================== + # PR closed (only set Done when merged) + # ================================================================== + if [ "$EVENT_NAME" = "pull_request" ]; then + if [ "$PR_MERGED" = "true" ]; then + echo "PR #$PR_NUMBER was merged — updating linked issues to Done" + + # The PR node itself is usually not in the board, but its linked + # issues are. Closing a PR does not auto-close issues, so we rely + # on "Linked pull requests" / closing keywords to find them. + # + # Step 1: query the PR for "closingIssuesReferences". + LINKED=$(gh api graphql \ + -f query=" + query { + node(id: \"$PR_NODE_ID\") { + ... on PullRequest { + closingIssuesReferences(first: 20) { + nodes { id number } + } + } + } + }" 2>/dev/null \ + | python3 -c " + import sys, json + data = json.load(sys.stdin) + nodes = data['data']['node']['closingIssuesReferences']['nodes'] + for n in nodes: + print(n['id'] + ' ' + str(n['number'])) + " 2>/dev/null || true) + + if [ -z "$LINKED" ]; then + echo "No linked closing issues found for PR #$PR_NUMBER." + # If the PR content itself is on the board, mark it Done. + item_id=$(get_item_id "$PR_NODE_ID") + if [ -n "$item_id" ]; then + set_done "$item_id" + fi + else + echo "$LINKED" | while read -r issue_node issue_num; do + [ -z "$issue_node" ] && continue + echo "Linked issue #$issue_num → marking Done" + item_id=$(get_item_id "$issue_node") + if [ -z "$item_id" ]; then + item_id=$(add_to_project "$issue_node") + fi + if [ -n "$item_id" ]; then + set_done "$item_id" + else + echo "⚠️ Could not resolve item id for issue #$issue_num" + fi + done + fi else - echo "⚠️ Status update failed: $UPDATE_RESULT" + echo "PR #$PR_NUMBER closed without merge — no status change." fi - } + exit 0 + fi - # ─── Main Logic ─── + # ================================================================== + # Issue events + # ================================================================== + if [ -z "$ISSUE_NODE_ID" ]; then + echo "No issue context (manual dispatch?). Exiting." + exit 0 + fi case "$EVENT" in opened|reopened) if [ "$ISSUE_STATE" = "open" ]; then echo "Adding opened/reopened issue #$ISSUE_NUMBER..." - add_to_project "$ISSUE_NODE_ID" - - # If reopened, find existing item and set status back - if [ "$EVENT" = "reopened" ]; then - ITEM_ID=$(find_item_id "$ISSUE_NODE_ID") - set_status "In Progress" "$ITEM_ID" + if ! add_to_project "$ISSUE_NODE_ID" >/dev/null; then + echo "::error::Failed to add issue #$ISSUE_NUMBER to project board" + exit 1 fi + else + echo "Issue #$ISSUE_NUMBER is not open. Skipping." fi ;; - closed) - echo "Issue #$ISSUE_NUMBER closed — setting Status to Done..." - ITEM_ID=$(find_item_id "$ISSUE_NODE_ID") - if [ -n "$ITEM_ID" ]; then - set_status "Done" "$ITEM_ID" + echo "Issue #$ISSUE_NUMBER closed — setting Status to Done" + item_id=$(get_item_id "$ISSUE_NODE_ID") + if [ -z "$item_id" ]; then + echo "Issue not yet on board — adding first." + item_id=$(add_to_project "$ISSUE_NODE_ID") + fi + if [ -n "$item_id" ]; then + set_done "$item_id" else - echo "⚠️ Issue not found in project board — adding first..." - add_to_project "$ISSUE_NODE_ID" - ITEM_ID=$(find_item_id "$ISSUE_NODE_ID") - set_status "Done" "$ITEM_ID" + echo "::error::Could not resolve project item for issue #$ISSUE_NUMBER" + exit 1 fi ;; - labeled) echo "Issue #$ISSUE_NUMBER labeled — no action needed" ;; + *) + echo "Unhandled event: $EVENT. No action." + ;; esac From 6793fc32ad87ed4b0ff532c30a1ccd7578b42c0a Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Tue, 7 Jul 2026 08:45:05 +0700 Subject: [PATCH 09/16] fix(llm): add conditional max_tokens parameter naming per provider (#340) LLM API providers use different parameter names for max output tokens: - OpenAI (legacy): max_tokens - OpenAI (new): max_completion_tokens - Anthropic: max_tokens - Google Gemini/Vertex: max_output_tokens cora-cli previously hardcoded 'max_tokens' everywhere, causing HTTP 400 errors ('Unsupported parameter: max_output_tokens') when providers don't accept it. Changes: - Add max_tokens_param to LlmSection config schema (supports 'auto' for provider-based detection, or explicit override) - Add resolve_max_tokens_param() in loader.rs for auto-detection logic - Replace hardcoded 'max_tokens' JSON key with dynamic key in both streaming and non-streaming LLM request paths - Add max_tokens_param to LLMConfig struct for runtime use - Display max_tokens_param in 'cora config' output Auto-detection maps: - gemini/google/vertex -> max_output_tokens - All other providers -> max_tokens Co-authored-by: ajianaz Co-authored-by: Hermes Agent --- src/commands/config_cmd.rs | 3 ++ src/config/loader.rs | 74 +++++++++++++++++++++++++++++++++ src/config/schema.rs | 67 ++++++++++++++++++++++++++++++ src/engine/llm.rs | 83 ++++++++++++++++++++++++++++---------- src/engine/types.rs | 4 ++ 5 files changed, 210 insertions(+), 21 deletions(-) diff --git a/src/commands/config_cmd.rs b/src/commands/config_cmd.rs index 612bfea..4ccca69 100644 --- a/src/commands/config_cmd.rs +++ b/src/commands/config_cmd.rs @@ -326,6 +326,9 @@ fn print_cora_file(cora: &CoraFile) { if let Some(v) = llm.max_tokens { parts.push(format!("max_tokens={}", v)); } + if let Some(ref v) = llm.max_tokens_param { + parts.push(format!("max_tokens_param={}", v)); + } if let Some(v) = llm.timeout { parts.push(format!("timeout={}", v)); } diff --git a/src/config/loader.rs b/src/config/loader.rs index d402128..53543b0 100644 --- a/src/config/loader.rs +++ b/src/config/loader.rs @@ -220,6 +220,22 @@ fn migrate_old_config() { } } +/// Resolve the max tokens parameter name based on provider and config setting. +/// +/// - If `config_value` is not `"auto"`, use it directly (explicit override). +/// - Otherwise, detect from provider name: +/// - `gemini`, `google`, `vertex` → `"max_output_tokens"` +/// - Everything else → `"max_tokens"` +pub fn resolve_max_tokens_param(provider: &str, config_value: &str) -> String { + match config_value { + v if v != "auto" => v.to_string(), + _ => match provider { + "gemini" | "google" | "vertex" => "max_output_tokens".to_string(), + _ => "max_tokens".to_string(), + }, + } +} + /// Load the full resolved config: defaults ← global config ← .cora.yaml ← CLI overrides. /// /// `cli_provider`, `cli_model`, `cli_api_key`, and `cli_format` are `None` @@ -374,6 +390,8 @@ pub fn build_llm_config( }) .unwrap_or_else(|| config.provider.base_url.clone()); + let max_tokens_param = resolve_max_tokens_param(&provider, &config.max_tokens_param); + Ok(LLMConfig { api_key, base_url, @@ -381,6 +399,7 @@ pub fn build_llm_config( provider, temperature: config.temperature, max_tokens: config.max_tokens, + max_tokens_param, timeout: config.timeout, }) } @@ -738,3 +757,58 @@ pub fn remove_provider_info() -> std::result::Result<(), CoraError> { Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn resolve_max_tokens_param_auto_gemini() { + assert_eq!( + resolve_max_tokens_param("gemini", "auto"), + "max_output_tokens" + ); + } + + #[test] + fn resolve_max_tokens_param_auto_google() { + assert_eq!( + resolve_max_tokens_param("google", "auto"), + "max_output_tokens" + ); + } + + #[test] + fn resolve_max_tokens_param_auto_vertex() { + assert_eq!( + resolve_max_tokens_param("vertex", "auto"), + "max_output_tokens" + ); + } + + #[test] + fn resolve_max_tokens_param_auto_openai() { + assert_eq!(resolve_max_tokens_param("openai", "auto"), "max_tokens"); + } + + #[test] + fn resolve_max_tokens_param_auto_anthropic() { + assert_eq!(resolve_max_tokens_param("anthropic", "auto"), "max_tokens"); + } + + #[test] + fn resolve_max_tokens_param_explicit_override() { + assert_eq!( + resolve_max_tokens_param("gemini", "max_tokens"), + "max_tokens" + ); + assert_eq!( + resolve_max_tokens_param("openai", "max_output_tokens"), + "max_output_tokens" + ); + assert_eq!( + resolve_max_tokens_param("anthropic", "max_completion_tokens"), + "max_completion_tokens" + ); + } +} diff --git a/src/config/schema.rs b/src/config/schema.rs index 5fdb9db..fba49e7 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -35,6 +35,8 @@ pub struct Config { pub temperature: f32, /// Max tokens for LLM responses. pub max_tokens: u32, + /// JSON parameter name for max tokens (e.g. "max_tokens" or "max_output_tokens"). + pub max_tokens_param: String, /// HTTP timeout in seconds for LLM requests. pub timeout: u64, /// Cache TTL in minutes for review caching. @@ -126,6 +128,7 @@ impl Default for Config { scan_system_prompt_file: None, temperature: 0.0, max_tokens: 4096, + max_tokens_param: "auto".to_string(), timeout: 600, cache_ttl: 1440, // 24h in minutes static_analysis: StaticAnalysisConfig::default(), @@ -297,6 +300,11 @@ pub struct LlmSection { pub temperature: Option, #[serde(skip_serializing_if = "Option::is_none")] pub max_tokens: Option, + /// Name of the max tokens parameter to use in API requests. + /// Supported values: `"auto"` (detect from provider), `"max_tokens"`, + /// `"max_output_tokens"`, `"max_completion_tokens"`. + #[serde(skip_serializing_if = "Option::is_none")] + pub max_tokens_param: Option, #[serde(skip_serializing_if = "Option::is_none")] pub timeout: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -449,6 +457,9 @@ impl CoraFile { if let Some(v) = llm.cache_ttl { config.cache_ttl = v; } + if let Some(ref v) = llm.max_tokens_param { + config.max_tokens_param = v.clone(); + } } if let Some(re) = &self.rules_engine { config.rules_config.enabled = re.enabled; @@ -1038,6 +1049,7 @@ llm: llm: Some(LlmSection { temperature: Some(0.7), max_tokens: None, + max_tokens_param: None, timeout: None, cache_ttl: None, }), @@ -1058,6 +1070,7 @@ llm: llm: Some(LlmSection { temperature: None, max_tokens: Some(2048), + max_tokens_param: None, timeout: None, cache_ttl: None, }), @@ -1074,6 +1087,7 @@ llm: llm: Some(LlmSection { temperature: None, max_tokens: None, + max_tokens_param: None, timeout: Some(300), cache_ttl: None, }), @@ -1090,6 +1104,7 @@ llm: llm: Some(LlmSection { temperature: Some(1.0), max_tokens: Some(16384), + max_tokens_param: None, timeout: Some(240), cache_ttl: Some(2880), }), @@ -1134,6 +1149,7 @@ provider: llm: Some(LlmSection { temperature: Some(0.5), max_tokens: Some(8192), + max_tokens_param: None, timeout: Some(60), cache_ttl: None, }), @@ -1278,4 +1294,55 @@ bundling: Some(crate::engine::bundling::GroupingStrategy::Smart) ); } + + // ─── max_tokens_param ─── + + #[test] + fn config_default_max_tokens_param() { + let cfg = Config::default(); + assert_eq!(cfg.max_tokens_param, "auto"); + } + + #[test] + fn merge_llm_max_tokens_param_explicit() { + let mut cfg = Config::default(); + let cora = CoraFile { + llm: Some(LlmSection { + max_tokens_param: Some("max_output_tokens".to_string()), + ..Default::default() + }), + ..Default::default() + }; + cora.merge_into(&mut cfg).unwrap(); + assert_eq!(cfg.max_tokens_param, "max_output_tokens"); + } + + #[test] + fn merge_llm_max_tokens_param_absent_leaves_default() { + let mut cfg = Config::default(); + let cora = CoraFile { + llm: Some(LlmSection { + temperature: Some(0.5), + ..Default::default() + }), + ..Default::default() + }; + cora.merge_into(&mut cfg).unwrap(); + assert_eq!(cfg.max_tokens_param, "auto"); + } + + #[test] + fn llm_section_yaml_roundtrip_with_max_tokens_param() { + let section = LlmSection { + temperature: Some(0.7), + max_tokens: Some(8192), + max_tokens_param: Some("max_output_tokens".to_string()), + timeout: Some(300), + cache_ttl: Some(60), + }; + let yaml = serde_yaml_ng::to_string(§ion).unwrap(); + let back: LlmSection = serde_yaml_ng::from_str(&yaml).unwrap(); + assert_eq!(back.max_tokens_param, Some("max_output_tokens".to_string())); + assert_eq!(back.max_tokens, Some(8192)); + } } diff --git a/src/engine/llm.rs b/src/engine/llm.rs index 8325054..cf2d5cc 100644 --- a/src/engine/llm.rs +++ b/src/engine/llm.rs @@ -54,7 +54,8 @@ struct ChatMessage { content: String, } -/// Request body for /chat/completions. +/// Request body for /chat/completions (kept for reference; unused after migration to dynamic json!). +#[allow(dead_code)] #[derive(Debug, Clone, Serialize)] struct ChatRequest { model: String, @@ -285,26 +286,19 @@ async fn chat_completion( )); } - let request = ChatRequest { - model: config.model.clone(), - messages: vec![ - ChatMessage { - role: "system".into(), - content: system_prompt.into(), - }, - ChatMessage { - role: "user".into(), - content: user_message.into(), - }, + let mut request = serde_json::json!({ + "model": config.model, + "messages": [ + { "role": "system", "content": system_prompt }, + { "role": "user", "content": user_message } ], - temperature: config.temperature, - max_tokens: config.max_tokens, - response_format: if response_format == "json_object" { - Some(serde_json::json!({"type": "json_object"})) - } else { - None - }, - }; + "temperature": config.temperature, + }); + request[config.max_tokens_param.clone()] = serde_json::json!(config.max_tokens); + + if response_format == "json_object" { + request["response_format"] = serde_json::json!({"type": "json_object"}); + } debug!(model = %config.model, url = %url, "sending LLM request"); @@ -541,12 +535,12 @@ async fn chat_completion_stream( { "role": "user", "content": user_message } ], "temperature": config.temperature, - "max_tokens": config.max_tokens, "stream": true, // Ask OpenAI-compatible providers to include token usage in the final // SSE chunk. Providers that don't recognise this field simply ignore it. "stream_options": { "include_usage": true } }); + request_body[config.max_tokens_param.clone()] = serde_json::json!(config.max_tokens); if response_format == "json_object" { request_body["response_format"] = serde_json::json!({"type": "json_object"}); @@ -1885,4 +1879,51 @@ mod tests { let messy = "hello\n\t world\n\n"; assert_eq!(preview_raw(messy), "hello world"); } + + // ─── max_tokens_param JSON key naming ─── + + #[test] + fn chat_request_uses_max_output_tokens() { + // Verify that when max_tokens_param is "max_output_tokens", the JSON + // body contains "max_output_tokens" (not "max_tokens") as the key. + let mut body = serde_json::json!({ + "model": "gemini-pro", + "messages": [ + { "role": "system", "content": "test" }, + { "role": "user", "content": "hello" } + ], + "temperature": 0.0, + }); + let param_name = "max_output_tokens"; + body[param_name] = serde_json::json!(4096); + + let serialized = serde_json::to_string(&body).unwrap(); + assert!( + serialized.contains(r#""max_output_tokens":4096"#), + "Expected max_output_tokens key in JSON, got: {serialized}" + ); + assert!( + !serialized.contains(r#""max_tokens":"#), + "Should NOT contain hardcoded max_tokens key, got: {serialized}" + ); + } + + #[test] + fn chat_request_uses_max_tokens_default() { + let mut body = serde_json::json!({ + "model": "gpt-4o-mini", + "messages": [ + { "role": "system", "content": "test" }, + { "role": "user", "content": "hello" } + ], + "temperature": 0.0, + }); + body["max_tokens"] = serde_json::json!(8192); + + let serialized = serde_json::to_string(&body).unwrap(); + assert!( + serialized.contains(r#""max_tokens":8192"#), + "Expected max_tokens key in JSON, got: {serialized}" + ); + } } diff --git a/src/engine/types.rs b/src/engine/types.rs index 326b71a..8a60ce1 100644 --- a/src/engine/types.rs +++ b/src/engine/types.rs @@ -306,6 +306,7 @@ mod tests { assert_eq!(cfg.provider, "openai"); assert_eq!(cfg.temperature, 0.0); assert_eq!(cfg.max_tokens, 4096); + assert_eq!(cfg.max_tokens_param, "max_tokens"); assert_eq!(cfg.timeout, 600); } @@ -423,6 +424,8 @@ pub struct LLMConfig { pub provider: String, pub temperature: f32, pub max_tokens: u32, + /// JSON parameter name for max tokens (resolved from config). + pub max_tokens_param: String, pub timeout: u64, } @@ -435,6 +438,7 @@ impl Default for LLMConfig { provider: "openai".to_string(), temperature: 0.0, max_tokens: 4096, + max_tokens_param: "max_tokens".to_string(), timeout: 600, } } From 1ee0f27dc19d752ffb209f39c3befaadc20bc58e Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Thu, 16 Jul 2026 07:57:30 +0700 Subject: [PATCH 10/16] perf: resolve scan/review pipeline performance bottlenecks (P2) (#338) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf: resolve scan/review pipeline performance bottlenecks (P2) Fix 10 performance issues — all 'repeated work in loops' patterns: #41 - Pre-compile regex patterns once per rule instead of per line #5 - Batch DB queries in handle_find_affected_tests (single IN clause) #13 - Batch symbol fetch for Affected command (single query) #14 - Prepare SQL statement once outside test-pattern loop #59 - Early cutoff in secrets scanner when max_findings reached #70 - Break out of all loops when symbol cap hit per file #67 - Cache file reads during sibling search in resolver #3 - Reuse single Tokio runtime for MCP tool calls (static LazyLock) #22 - Static LazyLock regexes in detect_function_entry #24 - Single transaction for prune_deleted instead of per-file Refs #335 * fix(ci): resolve clippy + security audit failures - clippy: remove redundant borrow in format! arg (llm.rs:456 &user_prompt) - security: bump anyhow 1.0.102 -> 1.0.103 (RUSTSEC-2026-0190, downcast_mut unsoundness) - security: bump crossbeam-epoch 0.9.18 -> 0.9.20 (RUSTSEC-2026-0204, fmt::Pointer null deref) Refs #335 --------- Co-authored-by: ajianaz --- Cargo.lock | 8 +-- src/engine/context/extraction.rs | 18 ++++-- src/engine/context/resolver.rs | 80 ++++++++++++++++++----- src/engine/llm.rs | 2 +- src/engine/rules/builtin.rs | 12 ++++ src/engine/rules/matching.rs | 15 +++-- src/engine/rules/mod.rs | 5 ++ src/engine/rules/types.rs | 22 ++++++- src/engine/secrets_scanner.rs | 12 ++++ src/engine/types.rs | 3 +- src/index/extract.rs | 25 ++++---- src/index/mod.rs | 17 +++-- src/main.rs | 47 ++++++++------ src/mcp/tools.rs | 105 +++++++++++++++++++------------ 14 files changed, 262 insertions(+), 109 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d1496d7..092ea67 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -84,9 +84,9 @@ dependencies = [ [[package]] name = "anyhow" -version = "1.0.102" +version = "1.0.103" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" +checksum = "2a4385e2e34eb35d6b3efe798b9eb88096925d87726c0798709bf56d9ed84af3" [[package]] name = "assert_cmd" @@ -334,9 +334,9 @@ dependencies = [ [[package]] name = "crossbeam-epoch" -version = "0.9.18" +version = "0.9.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +checksum = "2d6914041f254d6e9176c01941b21115dcfb7089e55135a35411081bd106ef3f" dependencies = [ "crossbeam-utils", ] diff --git a/src/engine/context/extraction.rs b/src/engine/context/extraction.rs index ad871c5..b24e874 100644 --- a/src/engine/context/extraction.rs +++ b/src/engine/context/extraction.rs @@ -274,14 +274,15 @@ pub fn extract_symbols_from_diff( continue; } + // Early cutoff: stop processing lines for this file once cap is reached + if file_symbol_count >= MAX_SYMBOLS_PER_FILE { + break; + } + let language = &file.language; let symbols = extract_symbols_from_line(&line.content, language); for sym in symbols { - if file_symbol_count >= MAX_SYMBOLS_PER_FILE { - break; - } - let key = format!("{sym}"); if seen.insert((key, file_path.to_string())) { all_symbols.push(ExtractedSymbol { @@ -291,8 +292,17 @@ pub fn extract_symbols_from_diff( raw: line.content.clone(), }); file_symbol_count += 1; + if file_symbol_count >= MAX_SYMBOLS_PER_FILE { + break; + } } } + if file_symbol_count >= MAX_SYMBOLS_PER_FILE { + break; + } + } + if file_symbol_count >= MAX_SYMBOLS_PER_FILE { + break; } } diff --git a/src/engine/context/resolver.rs b/src/engine/context/resolver.rs index 79df8bd..1ae197a 100644 --- a/src/engine/context/resolver.rs +++ b/src/engine/context/resolver.rs @@ -145,6 +145,9 @@ fn resolve_symbols( let mut entries = Vec::new(); let mut seen_files: HashMap> = HashMap::new(); // track line ranges per file + // File content cache to avoid re-reading the same files from disk + let mut file_cache: HashMap = HashMap::new(); + for sym in symbols { // Determine the language from the file extension let lang = Path::new(&sym.file) @@ -154,8 +157,12 @@ fn resolve_symbols( let resolved = match &sym.kind { SymbolKind::Import(path) => resolve_import(path, &sym.file, lang, project_root), - SymbolKind::FunctionCall(name) => resolve_function(name, &sym.file, lang, project_root), - SymbolKind::TypeRef(name) => resolve_type(name, &sym.file, lang, project_root), + SymbolKind::FunctionCall(name) => { + resolve_function(name, &sym.file, lang, project_root, &mut file_cache) + } + SymbolKind::TypeRef(name) => { + resolve_type(name, &sym.file, lang, project_root, &mut file_cache) + } }; for entry in resolved { @@ -344,6 +351,7 @@ fn resolve_function( source_file: &str, lang: &str, project_root: &Path, + file_cache: &mut HashMap, ) -> Vec { let mut entries = Vec::new(); @@ -370,7 +378,9 @@ fn resolve_function( continue; } - if let Some((start, end)) = find_fn_in_file(&path, name, "rs") { + if let Some((start, end)) = + find_fn_in_file_cached(&path, name, "rs", file_cache) + { entries.push(ContextEntry { file: rel_str, line_start: start, @@ -397,7 +407,9 @@ fn resolve_function( continue; } - if let Some((start, end)) = find_fn_in_file(&path, name, "py") { + if let Some((start, end)) = + find_fn_in_file_cached(&path, name, "py", file_cache) + { entries.push(ContextEntry { file: rel_str, line_start: start, @@ -421,7 +433,8 @@ fn resolve_function( continue; } - if let Some((start, end)) = find_fn_generic(&path, name) { + if let Some((start, end)) = find_fn_generic_cached(&path, name, file_cache) + { entries.push(ContextEntry { file: rel_str, line_start: start, @@ -445,6 +458,7 @@ fn resolve_type( source_file: &str, lang: &str, project_root: &Path, + file_cache: &mut HashMap, ) -> Vec { let search_dir = Path::new(source_file).parent().unwrap_or(Path::new("")); let mut entries = Vec::new(); @@ -466,7 +480,8 @@ fn resolve_type( continue; } - if let Some((start, end)) = find_pattern_in_file(&path, &pattern) { + if let Some((start, end)) = find_pattern_in_file_cached(&path, &pattern, file_cache) + { entries.push(ContextEntry { file: rel_str, line_start: start, @@ -482,37 +497,68 @@ fn resolve_type( entries } -/// Find a function definition in a file and return (start_line, end_line). -fn find_fn_in_file(path: &Path, name: &str, lang: &str) -> Option<(u32, u32)> { - let content = std::fs::read_to_string(path).ok()?; +/// Cached version of `find_fn_in_file` — avoids re-reading files from disk. +fn find_fn_in_file_cached( + path: &Path, + name: &str, + lang: &str, + file_cache: &mut HashMap, +) -> Option<(u32, u32)> { + let content = get_file_cached(path, file_cache)?; let pattern = match lang { "rs" => format!("fn {name}"), "py" => format!("def {name}"), - _ => return find_fn_generic(path, name), - }; - - let max_lines = match lang { - "rs" => MAX_FN_LINES, - "py" => MAX_FN_LINES, - _ => MAX_FN_LINES, + _ => return find_fn_generic_cached(path, name, file_cache), }; - find_pattern_with_body(&content, &pattern, max_lines) + find_pattern_with_body(&content, &pattern, MAX_FN_LINES) } /// Generic function search (for languages without specific patterns). +#[allow(dead_code)] fn find_fn_generic(path: &Path, name: &str) -> Option<(u32, u32)> { let content = std::fs::read_to_string(path).ok()?; find_pattern_with_body(&content, &format!("fn {name}"), MAX_FN_LINES) } +/// Cached version of `find_fn_generic`. +fn find_fn_generic_cached( + path: &Path, + name: &str, + file_cache: &mut HashMap, +) -> Option<(u32, u32)> { + let content = get_file_cached(path, file_cache)?; + find_pattern_with_body(&content, &format!("fn {name}"), MAX_FN_LINES) +} + /// Find a pattern (like `struct Foo`) and determine its extent. +#[allow(dead_code)] fn find_pattern_in_file(path: &Path, pattern: &str) -> Option<(u32, u32)> { let content = std::fs::read_to_string(path).ok()?; find_pattern_with_body(&content, pattern, MAX_TYPE_LINES) } +/// Cached version of `find_pattern_in_file`. +fn find_pattern_in_file_cached( + path: &Path, + pattern: &str, + file_cache: &mut HashMap, +) -> Option<(u32, u32)> { + let content = get_file_cached(path, file_cache)?; + find_pattern_with_body(&content, pattern, MAX_TYPE_LINES) +} + +/// Read a file from disk, using the cache if available. +fn get_file_cached(path: &Path, cache: &mut HashMap) -> Option { + if let Some(content) = cache.get(path) { + return Some(content.clone()); + } + let content = std::fs::read_to_string(path).ok()?; + cache.insert(path.to_path_buf(), content.clone()); + Some(content) +} + /// Find a pattern in content and estimate the block extent by counting braces/indents. fn find_pattern_with_body(content: &str, pattern: &str, max_lines: usize) -> Option<(u32, u32)> { let mut start_line: Option = None; diff --git a/src/engine/llm.rs b/src/engine/llm.rs index cf2d5cc..9cc2a51 100644 --- a/src/engine/llm.rs +++ b/src/engine/llm.rs @@ -453,7 +453,7 @@ pub async fn review_diff( "{}\n\nIMPORTANT: Your response MUST contain only valid JSON. \ Ensure all strings use proper JSON escape sequences. \ Do NOT use raw backslashes in string values.", - &user_prompt + user_prompt ); let (retry_raw, retry_usage) = chat_completion( llm_config, diff --git a/src/engine/rules/builtin.rs b/src/engine/rules/builtin.rs index 1b682bc..96011cb 100644 --- a/src/engine/rules/builtin.rs +++ b/src/engine/rules/builtin.rs @@ -14,6 +14,7 @@ pub fn builtin_rules() -> Vec { message: "Possible hardcoded secret/credential detected. Use environment variables or a secrets manager.".to_string(), languages: vec!["all".to_string()], exclude: vec!["rules/".to_string(), "tests/".to_string(), "test/".to_string()], + ..Default::default() }, CustomRule { id: "sec-sql-concat".to_string(), @@ -23,6 +24,7 @@ pub fn builtin_rules() -> Vec { message: "Possible SQL injection via string concatenation in query. Use parameterized queries.".to_string(), languages: vec!["all".to_string()], exclude: vec!["rules/".to_string(), "tests/".to_string(), "test/".to_string()], + ..Default::default() }, CustomRule { id: "sec-hardcoded-url".to_string(), @@ -32,6 +34,7 @@ pub fn builtin_rules() -> Vec { message: "Insecure HTTP URL detected (not https). Use HTTPS for all external connections.".to_string(), languages: vec!["all".to_string()], exclude: vec!["rules/".to_string(), "tests/".to_string(), "test/".to_string()], + ..Default::default() }, CustomRule { id: "sec-tls-disabled".to_string(), @@ -40,6 +43,7 @@ pub fn builtin_rules() -> Vec { message: "TLS verification disabled. This allows man-in-the-middle attacks.".to_string(), languages: vec!["rs".to_string(), "py".to_string(), "go".to_string()], exclude: vec!["rules/".to_string(), "tests/".to_string(), "test/".to_string()], + ..Default::default() }, // --- Bugs --- CustomRule { @@ -49,6 +53,7 @@ pub fn builtin_rules() -> Vec { message: "Use of `.unwrap()` can panic in production. Handle the error properly.".to_string(), languages: vec!["rs".to_string()], exclude: vec!["tests/".to_string(), "test/".to_string()], + ..Default::default() }, CustomRule { id: "bug-expect".to_string(), @@ -58,6 +63,7 @@ pub fn builtin_rules() -> Vec { message: "Use of `.expect()` can panic in production. Consider proper error handling.".to_string(), languages: vec!["rs".to_string()], exclude: vec!["tests/".to_string(), "test/".to_string()], + ..Default::default() }, CustomRule { id: "bug-println".to_string(), @@ -66,6 +72,7 @@ pub fn builtin_rules() -> Vec { message: "Debug output macro found. Remove `println!`/`dbg!`/`print!` before merging.".to_string(), languages: vec!["rs".to_string()], exclude: vec!["tests/".to_string(), "test/".to_string()], + ..Default::default() }, CustomRule { id: "bug-todo".to_string(), @@ -74,6 +81,7 @@ pub fn builtin_rules() -> Vec { message: "TODO/FIXME/HACK/XXX comment found. Consider resolving before merge.".to_string(), languages: vec!["all".to_string()], exclude: vec![], + ..Default::default() }, CustomRule { id: "bug-console-log".to_string(), @@ -82,6 +90,7 @@ pub fn builtin_rules() -> Vec { message: "Console logging statement found. Remove before merging to production.".to_string(), languages: vec!["js".to_string(), "ts".to_string()], exclude: vec!["tests/".to_string(), "test/".to_string()], + ..Default::default() }, CustomRule { id: "bug-hardcoded-port".to_string(), @@ -90,6 +99,7 @@ pub fn builtin_rules() -> Vec { message: "Hardcoded port number detected. Consider using environment variables or config.".to_string(), languages: vec!["all".to_string()], exclude: vec![], + ..Default::default() }, // --- Quality --- CustomRule { @@ -99,6 +109,7 @@ pub fn builtin_rules() -> Vec { message: "Error result discarded with `let _ =`. Consider handling the error explicitly.".to_string(), languages: vec!["all".to_string()], exclude: vec![], + ..Default::default() }, CustomRule { id: "qual-clone".to_string(), @@ -107,6 +118,7 @@ pub fn builtin_rules() -> Vec { message: "Use of `.clone()` detected. Consider borrowing or ownership transfer.".to_string(), languages: vec!["rs".to_string()], exclude: vec![], + ..Default::default() }, ] } diff --git a/src/engine/rules/matching.rs b/src/engine/rules/matching.rs index 7b5dd25..444b9ba 100644 --- a/src/engine/rules/matching.rs +++ b/src/engine/rules/matching.rs @@ -33,10 +33,10 @@ pub fn matches_exclude(rule: &CustomRule, file_path: &str) -> bool { /// Check if a rule's regex pattern matches a line of code. pub fn match_rule_against_line(rule: &CustomRule, line: &str) -> bool { - match regex::Regex::new(&rule.pattern) { - Ok(re) => re.is_match(line), - Err(e) => { - debug!(rule_id = %rule.id, error = %e, "invalid regex in rule, skipping"); + match &rule.compiled_pattern { + Some(re) => re.is_match(line), + None => { + debug!(rule_id = %rule.id, "regex not compiled, skipping"); false } } @@ -112,14 +112,17 @@ mod tests { use crate::engine::Severity; fn make_rule(pattern: &str, languages: &[&str], exclude: &[&str]) -> CustomRule { - CustomRule { + let mut rule = CustomRule { id: "test-rule".to_string(), pattern: pattern.to_string(), severity: Severity::Minor, message: "test".to_string(), languages: languages.iter().map(|s| s.to_string()).collect(), exclude: exclude.iter().map(|s| s.to_string()).collect(), - } + compiled_pattern: None, + }; + rule.ensure_compiled(); + rule } #[test] diff --git a/src/engine/rules/mod.rs b/src/engine/rules/mod.rs index c8028f8..f4f7bd6 100644 --- a/src/engine/rules/mod.rs +++ b/src/engine/rules/mod.rs @@ -32,6 +32,11 @@ pub fn run_rules(chunks: &[FileChunk], config: &RulesConfig) -> Vec let mut all_rules = builtin::builtin_rules(); all_rules.extend(config.custom_rules.clone()); + // Compile regex patterns once before the matching loop (Fix #41) + for rule in &mut all_rules { + rule.ensure_compiled(); + } + debug!( rule_count = all_rules.len(), "running rules against diff chunks" diff --git a/src/engine/rules/types.rs b/src/engine/rules/types.rs index dae6d8d..59d74ba 100644 --- a/src/engine/rules/types.rs +++ b/src/engine/rules/types.rs @@ -25,7 +25,7 @@ impl Default for RulesConfig { } /// A user-defined or built-in rule definition. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct CustomRule { /// Unique rule identifier (e.g., `"sec-hardcoded-secret"`). pub id: String, @@ -39,6 +39,26 @@ pub struct CustomRule { pub languages: Vec, /// Glob patterns for file paths to exclude from this rule. pub exclude: Vec, + /// Pre-compiled regex for the pattern (populated after rule assembly). + #[serde(skip, default)] + pub compiled_pattern: Option, +} + +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. + pub fn ensure_compiled(&mut self) -> bool { + if self.compiled_pattern.is_some() { + return true; + } + match regex::Regex::new(&self.pattern) { + Ok(re) => { + self.compiled_pattern = Some(re); + true + } + Err(_) => false, + } + } } /// A single finding produced by a rule. diff --git a/src/engine/secrets_scanner.rs b/src/engine/secrets_scanner.rs index 9d9a881..398ab55 100644 --- a/src/engine/secrets_scanner.rs +++ b/src/engine/secrets_scanner.rs @@ -149,10 +149,22 @@ pub fn scan_secrets(chunks: &[FileChunk], max_findings: usize) -> Vec= max_findings { + break; + } } } + if findings.len() >= max_findings { + break; + } + } + if findings.len() >= max_findings { + break; } } + if findings.len() >= max_findings { + break; + } } // Sort Critical first diff --git a/src/engine/types.rs b/src/engine/types.rs index 8a60ce1..861e513 100644 --- a/src/engine/types.rs +++ b/src/engine/types.rs @@ -2,9 +2,10 @@ use serde::{Deserialize, Serialize}; use std::fmt; /// Issue severity levels -#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize, Default)] #[serde(rename_all = "lowercase")] pub enum Severity { + #[default] Critical, Major, Minor, diff --git a/src/index/extract.rs b/src/index/extract.rs index 8476615..0b013f2 100644 --- a/src/index/extract.rs +++ b/src/index/extract.rs @@ -419,29 +419,32 @@ fn detect_function_entry(line: &str, language: &str) -> Option { match language { "rs" => { - // pub fn name( or fn name( - let re = regex::Regex::new(r"(?:pub\s+)?(?:async\s+)?fn\s+(\w+)\s*[(<]").unwrap(); - re.captures(trimmed) + static RE: LazyLock = + LazyLock::new(|| Regex::new(r"(?:pub\s+)?(?:async\s+)?fn\s+(\w+)\s*[(<]").unwrap()); + RE.captures(trimmed) .map(|c| c.get(1).unwrap().as_str().to_string()) } "go" => { - let re = regex::Regex::new(r"func\s+(?:\([^)]+\)\s+)?(\w+)\s*\(").unwrap(); - re.captures(trimmed) + static RE: LazyLock = + LazyLock::new(|| Regex::new(r"func\s+(?:\([^)]+\)\s+)?(\w+)\s*\(").unwrap()); + RE.captures(trimmed) .map(|c| c.get(1).unwrap().as_str().to_string()) } "c" | "cpp" | "h" | "hpp" => { - let re = regex::Regex::new(r"[\w:*]+\s+(\w+)\s*\([^;]*\)\s*\{").unwrap(); - re.captures(trimmed) + static RE: LazyLock = + LazyLock::new(|| Regex::new(r"[\w:*]+\s+(\w+)\s*\([^;]*\)\s*\{").unwrap()); + RE.captures(trimmed) .map(|c| c.get(1).unwrap().as_str().to_string()) } "java" | "kt" => { - let re = regex::Regex::new(r"\b(\w+)\s*\([^)]*\)\s*\{").unwrap(); - re.captures(trimmed) + static RE: LazyLock = + LazyLock::new(|| Regex::new(r"\b(\w+)\s*\([^)]*\)\s*\{").unwrap()); + RE.captures(trimmed) .map(|c| c.get(1).unwrap().as_str().to_string()) } "py" => { - let re = regex::Regex::new(r"def\s+(\w+)").unwrap(); - re.captures(trimmed) + static RE: LazyLock = LazyLock::new(|| Regex::new(r"def\s+(\w+)").unwrap()); + RE.captures(trimmed) .map(|c| c.get(1).unwrap().as_str().to_string()) } _ => None, diff --git a/src/index/mod.rs b/src/index/mod.rs index 6fe76a5..d662ecd 100644 --- a/src/index/mod.rs +++ b/src/index/mod.rs @@ -243,18 +243,23 @@ pub fn prune_deleted(conn: &Connection, root: &Path) -> anyhow::Result { .filter_map(|r| r.ok()) .collect(); - for path in &paths { - let full = root.join(path); - if !full.exists() { - let tx = conn.unchecked_transaction()?; + // Batch all deletes in a single transaction instead of per-file + let to_prune: Vec<&String> = paths + .iter() + .filter(|path| !root.join(path).exists()) + .collect(); + + if !to_prune.is_empty() { + let tx = conn.unchecked_transaction()?; + for path in &to_prune { tx.execute( "DELETE FROM symbols WHERE file = ?1", rusqlite::params![path], )?; tx.execute("DELETE FROM files WHERE path = ?1", rusqlite::params![path])?; - tx.commit()?; - deleted += 1; } + tx.commit()?; + deleted = to_prune.len(); } if deleted > 0 { diff --git a/src/main.rs b/src/main.rs index 3190f8e..522320d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -842,29 +842,40 @@ async fn main() -> Result<()> { std::collections::HashSet::new(); // Strategy 1: Find symbols in changed files, then find callers that are in test files - for file in &changed { - // Get all symbols defined in this file - let symbols_in_file: Vec = { - let mut stmt = - conn.prepare("SELECT DISTINCT name FROM symbols WHERE file = ?1")?; - let rows = - stmt.query_map(rusqlite::params![file], |row| row.get::<_, String>(0))?; - rows.filter_map(|r| r.ok()).collect() - }; - - // For each symbol, find callers - for sym_name in &symbols_in_file { - let callers = index::graph::find_callers(&conn, sym_name, 100)?; - for caller in callers { - // Check if caller file looks like a test file - if patterns.iter().any(|p| caller.file.contains(p.as_str())) { - affected_tests.insert(caller.file.clone()); + // Batch: fetch all symbols for all changed files in a single query + let all_symbols: Vec = { + let placeholders: String = + changed.iter().map(|_| "?").collect::>().join(","); + let sql = + format!("SELECT DISTINCT name FROM symbols WHERE file IN ({placeholders})"); + let mut stmt = conn.prepare(&sql)?; + let params: Vec<&dyn rusqlite::types::ToSql> = changed + .iter() + .map(|f| f as &dyn rusqlite::types::ToSql) + .collect(); + let rows = stmt.query_map(params.as_slice(), |row| row.get::<_, String>(0))?; + rows.filter_map(|r| r.ok()).collect() + }; + + // Deduplicate symbols and resolve callers with a single set-based query + { + let mut seen_syms: std::collections::HashSet = + std::collections::HashSet::new(); + for sym_name in all_symbols { + if seen_syms.insert(sym_name.clone()) { + let callers = index::graph::find_callers(&conn, &sym_name, 100)?; + for caller in callers { + if patterns.iter().any(|p| caller.file.contains(p.as_str())) { + affected_tests.insert(caller.file.clone()); + } } } } } // Strategy 2: Direct test file name convention (mod_test.rs, foo_test.go) + // Prepare statement once before the loop + let mut stmt = conn.prepare("SELECT DISTINCT path FROM files WHERE path LIKE ?1")?; for file in &changed { // For Rust: src/foo.rs → tests/foo.rs or src/foo.rs → src/foo_test.rs let stem = file @@ -885,8 +896,6 @@ async fn main() -> Result<()> { format!("{stem}.spec.ts"), ]; for tp in &test_patterns { - let mut stmt = - conn.prepare("SELECT DISTINCT path FROM files WHERE path LIKE ?1")?; let pattern = format!("%{tp}"); let rows = stmt.query_map(rusqlite::params![pattern], |row| row.get::<_, String>(0))?; diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index 760b0c8..f8b6b66 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -2,6 +2,8 @@ //! //! Each handler takes JSON params and returns a ToolResult. +use std::sync::LazyLock; + use crate::engine::diff_parser; use crate::engine::profiles; use crate::engine::rules; @@ -10,6 +12,10 @@ use crate::engine::security_scanner; use super::protocol::{Tool, ToolResult}; +/// Shared Tokio runtime for MCP tool handlers — created once, reused across calls. +static MCP_RUNTIME: LazyLock = + LazyLock::new(|| tokio::runtime::Runtime::new().expect("Failed to create MCP tokio runtime")); + /// List all available MCP tools. pub fn list_tools() -> Vec { vec![ @@ -525,32 +531,44 @@ fn handle_find_affected_tests(params: &serde_json::Value) -> ToolResult { let patterns = ["test", "spec", "_test", "_spec"]; let mut affected: std::collections::HashSet = std::collections::HashSet::new(); - for file in &files { - // Strategy 1: call graph - let symbols_in_file: Vec = { - let mut stmt = match conn.prepare("SELECT DISTINCT name FROM symbols WHERE file = ?1") { - Ok(s) => s, - Err(e) => return ToolResult::error(format!("DB error: {e}")), - }; - let rows = match stmt.query_map(rusqlite::params![file], |row| row.get::<_, String>(0)) - { - Ok(r) => r, - Err(e) => return ToolResult::error(format!("DB error: {e}")), - }; - rows.filter_map(|r| r.ok()).collect() + // Batch fetch all symbols for all files in a single query + let all_symbols: Vec = { + let placeholders = files.iter().map(|_| "?").collect::>().join(","); + let sql = format!("SELECT DISTINCT name FROM symbols WHERE file IN ({placeholders})"); + let mut stmt = match conn.prepare(&sql) { + Ok(s) => s, + Err(e) => return ToolResult::error(format!("DB error: {e}")), }; + let params: Vec<&dyn rusqlite::types::ToSql> = files + .iter() + .map(|f| f as &dyn rusqlite::types::ToSql) + .collect(); + let rows = match stmt.query_map(params.as_slice(), |row| row.get::<_, String>(0)) { + Ok(r) => r, + Err(e) => return ToolResult::error(format!("DB error: {e}")), + }; + rows.filter_map(|r| r.ok()).collect() + }; - for sym_name in &symbols_in_file { - if let Ok(callers) = crate::index::graph::find_callers(&conn, sym_name, 100) { - for caller in callers { - if patterns.iter().any(|p| caller.file.contains(*p)) { - affected.insert(caller.file.clone()); + // Deduplicate and traverse call graph once + { + let mut seen_syms: std::collections::HashSet = std::collections::HashSet::new(); + for sym_name in &all_symbols { + if seen_syms.insert(sym_name.clone()) { + if let Ok(callers) = crate::index::graph::find_callers(&conn, sym_name, 100) { + for caller in callers { + if patterns.iter().any(|p| caller.file.contains(*p)) { + affected.insert(caller.file.clone()); + } } } } } + } - // Strategy 2: naming convention + // Strategy 2: naming convention — batch all test name candidates + let mut test_names: Vec = Vec::new(); + for file in &files { let stem = file .rsplit('/') .next() @@ -558,25 +576,39 @@ fn handle_find_affected_tests(params: &serde_json::Value) -> ToolResult { .rsplit('.') .next() .unwrap_or(""); - let test_names = [ + test_names.extend_from_slice(&[ format!("{stem}_test.rs"), format!("tests/{stem}.rs"), format!("{stem}_test.go"), format!("test_{stem}.py"), format!("{stem}.test.ts"), format!("{stem}.spec.ts"), - ]; - for tn in &test_names { - if let Ok(mut stmt) = conn.prepare("SELECT DISTINCT path FROM files WHERE path LIKE ?1") - { - let pattern = format!("%{tn}"); - if let Ok(rows) = - stmt.query_map(rusqlite::params![pattern], |row| row.get::<_, String>(0)) - { - for row in rows.map_while(Result::ok) { - affected.insert(row); - } - } + ]); + } + + // Query with a single LIKE batch + { + let sql = format!( + "SELECT DISTINCT path FROM files WHERE path LIKE '%' || ?1 OR {}", + test_names + .iter() + .enumerate() + .skip(1) + .map(|(i, _)| format!("path LIKE '%' || ?{}", i + 1)) + .collect::>() + .join(" OR ") + ); + let mut stmt = match conn.prepare(&sql) { + Ok(s) => s, + Err(e) => return ToolResult::error(format!("DB error: {e}")), + }; + let params: Vec<&dyn rusqlite::types::ToSql> = test_names + .iter() + .map(|t| t as &dyn rusqlite::types::ToSql) + .collect(); + if let Ok(rows) = stmt.query_map(params.as_slice(), |row| row.get::<_, String>(0)) { + for row in rows.map_while(Result::ok) { + affected.insert(row); } } } @@ -639,13 +671,8 @@ fn handle_review_diff(params: &serde_json::Value) -> ToolResult { } }; - // Run review synchronously - let rt = match tokio::runtime::Runtime::new() { - Ok(rt) => rt, - Err(e) => return ToolResult::error(format!("Failed to create runtime: {e}")), - }; - - let result = rt.block_on(crate::engine::review::review_diff_with_cache( + // Run review synchronously using the shared runtime + let result = MCP_RUNTIME.block_on(crate::engine::review::review_diff_with_cache( &config, &llm_config, diff, From 8808b0d73350ead1f6692355daf65f7ba8f7872a Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Thu, 16 Jul 2026 08:33:00 +0700 Subject: [PATCH 11/16] fix(ci): project-sync should not fail when merged PR has no closing keyword (#341) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A merged PR that references issues via "Refs #N" (not "Closes #N") has no closingIssuesReferences — a legitimate non-error outcome. Previously the script could abort with exit 1 under 'set -e' when get_item_id() hit an unexpected GraphQL response shape (e.g. node: null), surfacing as a red 'sync' check on otherwise-green PRs (e.g. PR #338 merge). - Make get_item_id() and the LINKED query python defensive against null/unexpected shapes - Guard the empty-LINKED branch call sites with '|| true' so a missing board item never aborts the job --- .github/workflows/project-sync.yml | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/.github/workflows/project-sync.yml b/.github/workflows/project-sync.yml index c081110..576e357 100644 --- a/.github/workflows/project-sync.yml +++ b/.github/workflows/project-sync.yml @@ -121,8 +121,10 @@ jobs: | python3 -c " import sys, json data = json.load(sys.stdin) + node = (data.get('data') or {}).get('node') or {} + items = (node.get('items') or {}).get('nodes') or [] target = '$content_node_id' - for n in data['data']['node']['items']['nodes']: + for n in items: c = n.get('content') or {} if c.get('id') == target: print(n['id']) @@ -176,17 +178,21 @@ jobs: | python3 -c " import sys, json data = json.load(sys.stdin) - nodes = data['data']['node']['closingIssuesReferences']['nodes'] + node = (data.get('data') or {}).get('node') or {} + nodes = (node.get('closingIssuesReferences') or {}).get('nodes') or [] for n in nodes: print(n['id'] + ' ' + str(n['number'])) " 2>/dev/null || true) if [ -z "$LINKED" ]; then echo "No linked closing issues found for PR #$PR_NUMBER." - # If the PR content itself is on the board, mark it Done. - item_id=$(get_item_id "$PR_NODE_ID") + # A merged PR that references issues via "Refs #N" (rather than + # a closing keyword) legitimately has no closing references — + # this is not an error. Best-effort: mark the PR itself Done if + # it happens to be on the board. + item_id=$(get_item_id "$PR_NODE_ID" || true) if [ -n "$item_id" ]; then - set_done "$item_id" + set_done "$item_id" || true fi else echo "$LINKED" | while read -r issue_node issue_num; do From 3ac8a7721741d5dbff330b5d821975f96c6e3d0f Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Thu, 16 Jul 2026 08:55:46 +0700 Subject: [PATCH 12/16] fix(config): add config validation + deny_unknown_fields (#334 P3-core) (#342) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the P3-core items from #334 — config values and profile values were accepted without semantic checks, so typos and out-of-range values propagated silently to runtime. #94 — Config::validate() 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 - provider.base_url must have an http(s)/ws/unix scheme - multiple errors aggregated into one message #81 — Profile::validate(): focus weight 1-10, recognized action/tone/detail_level #57 — CategoryAction enum (case-insensitive deserialize): unknown values like 'blok' now fail loudly instead of silently becoming blocking #58 — evaluate() forces Pass when enabled=false (disabled gate never fails) #80 — deny_unknown_fields on all config sections: misspelled YAML keys (e.g. 'quailty_gate', 'temprature') rejected at parse time 24 new tests covering validation acceptance/rejection and deny_unknown_fields. All 637 tests pass; clippy + fmt clean. Refs #334 --- CHANGELOG.md | 11 +++ docs/changelog.md | 8 ++ src/config/loader.rs | 4 + src/config/schema.rs | 198 +++++++++++++++++++++++++++++++++++++ src/engine/profiles.rs | 105 ++++++++++++++++++++ src/engine/quality_gate.rs | 128 ++++++++++++++++++++---- 6 files changed, 436 insertions(+), 18 deletions(-) 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()); + } } From bd89814a17e8e6aeb17a9ccd4d61ce7abd9a338d Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Thu, 16 Jul 2026 09:16:30 +0700 Subject: [PATCH 13/16] fix(review): suppress findings inside Markdown fenced code blocks (#329) (#343) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Markdown files (.md/.mdx/.markdown) frequently contain fenced code blocks that are documentation examples, not executable code. Treating their contents like real source code produced false positives — e.g. a `git push` inside a fenced bash block flagged as SQL injection (#329). New module `engine::markdown` detects fenced code blocks (triple-backtick / triple-tilde) within a diff chunk and exposes which new-file line numbers fall inside them. A new filter `apply_markdown_code_block_filter()` drops findings (from all sources: security/secrets/rules scanners + LLM) whose file:line lands inside a code block of a markdown file. Fence state is tracked across full hunk context (Add + Context lines, skipping Removed lines) so it works even when only the body of a code block was edited — the opening fence does not need to be part of the diff. Findings without a resolvable line number are kept (safe default). 11 new tests (7 unit for fence detection + 4 integration for the filter, including the exact #329 git-push/SQL-injection scenario). All 648 tests pass; clippy + fmt + audit clean. Refs #329 --- CHANGELOG.md | 4 + docs/changelog.md | 4 + src/engine/markdown.rs | 208 +++++++++++++++++++++++++++++++++++ src/engine/mod.rs | 1 + src/engine/review.rs | 241 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 458 insertions(+) create mode 100644 src/engine/markdown.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c0e9e8..4e451d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — Markdown False Positives (#329) + +- **Findings inside Markdown fenced code blocks are now suppressed.** Code blocks (\`\`\` / `~~~`) in `.md`/`.mdx`/`.markdown` files are documentation examples, not executable code. A `git push` inside a ` ```bash ` block is no longer flagged as SQL injection. The filter covers all finding sources (security/secrets/rules scanners + LLM) and uses full hunk context (Add + Context lines) to track fence state, so it works even when only the code-block body was edited. + ### 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. diff --git a/docs/changelog.md b/docs/changelog.md index 081f804..d2115f2 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — Markdown False Positives (#329) + +- **Findings inside Markdown fenced code blocks are now suppressed.** A `git push` inside a ` ```bash ` block in an `.md` file is no longer flagged as SQL injection. Covers all finding sources (security/secrets/rules + LLM); tracks fence state across full hunk context (Add + Context lines). + ### 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. diff --git a/src/engine/markdown.rs b/src/engine/markdown.rs new file mode 100644 index 0000000..1b5aa96 --- /dev/null +++ b/src/engine/markdown.rs @@ -0,0 +1,208 @@ +//! Markdown awareness for the review/scan pipeline. +//! +//! Markdown files (`.md`, `.mdx`, `.markdown`) frequently contain fenced code +//! blocks that are **documentation examples**, not executable code. Treating +//! their contents like real source code produces false positives — e.g. a +//! `git push` line inside a ` ```bash ` block flagged as SQL injection (#329). +//! +//! This module detects fenced code blocks (` ``` ` / `~~~`) within a diff chunk +//! and exposes which new-file line numbers fall inside them, so findings can be +//! filtered out. + +use std::collections::HashSet; + +use crate::engine::diff_parser::{DiffLineType, FileChunk}; + +/// True if the path looks like a Markdown documentation file. +pub fn is_markdown(path: &str) -> bool { + let lower = path.to_lowercase(); + lower.ends_with(".md") || lower.ends_with(".mdx") || lower.ends_with(".markdown") +} + +/// Compute the set of new-file line numbers that fall **inside** fenced code +/// blocks for a single diff chunk. +/// +/// Only lines present in the *new* file are considered (Add + Context). Removed +/// lines are skipped so they cannot corrupt fence state. The opening fence does +/// not need to be part of the diff itself — context lines carry it too, which +/// keeps this reliable when only the body of a code block was edited. +/// +/// Fence state is tracked across all hunks in the chunk in document order. This +/// is exact when a code block lives within contiguous lines and a best-effort +/// approximation when a fence spans non-contiguous hunks. +pub fn lines_inside_code_blocks(chunk: &FileChunk) -> HashSet { + let mut inside = HashSet::new(); + let mut in_fence = false; + + for hunk in &chunk.chunks { + for line in &hunk.lines { + // Removed lines don't exist in the new file — skip to keep fence + // state faithful to the post-change document. + if line.line_type == DiffLineType::Remove { + continue; + } + + let trimmed = line.content.trim_start(); + let is_fence = trimmed.starts_with("```") || trimmed.starts_with("~~~"); + if is_fence { + in_fence = !in_fence; + continue; + } + + if in_fence { + if let Some(ln) = line.new_line_no { + if ln > 0 { + inside.insert(ln); + } + } + } + } + } + + inside +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::engine::diff_parser::{DiffHunk, DiffLine}; + + fn chunk(file: &str, lines: &[(DiffLineType, &str, u32)]) -> FileChunk { + FileChunk { + old_path: None, + new_path: Some(file.to_string()), + language: "markdown".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: lines.len() as u32, + header: String::new(), + lines: lines + .iter() + .map(|(lt, content, ln)| DiffLine { + line_type: *lt, + content: content.to_string(), + old_line_no: None, + new_line_no: Some(*ln), + }) + .collect(), + }], + is_binary: false, + is_deleted: false, + is_new: false, + } + } + + #[test] + fn detects_markdown_extensions() { + assert!(is_markdown("README.md")); + assert!(is_markdown("docs/guide.MDX")); + assert!(is_markdown("notes.markdown")); + assert!(!is_markdown("src/main.rs")); + assert!(!is_markdown("config.yaml")); + } + + #[test] + fn lines_inside_backtick_fence() { + let c = chunk( + "doc.md", + &[ + (DiffLineType::Add, "Intro", 1), + (DiffLineType::Add, "```bash", 2), + (DiffLineType::Add, "git push origin vX.Y.Z", 3), + (DiffLineType::Add, "git tag vX.Y.Z", 4), + (DiffLineType::Add, "```", 5), + (DiffLineType::Add, "Done.", 6), + ], + ); + let inside = lines_inside_code_blocks(&c); + assert!(inside.contains(&3)); + assert!(inside.contains(&4)); + assert!(!inside.contains(&1)); + assert!(!inside.contains(&6)); + // Fence lines themselves are not "inside". + assert!(!inside.contains(&2)); + assert!(!inside.contains(&5)); + } + + #[test] + fn lines_inside_tilde_fence() { + let c = chunk( + "doc.md", + &[ + (DiffLineType::Add, "~~~sql", 1), + (DiffLineType::Add, "SELECT 1;", 2), + (DiffLineType::Add, "~~~", 3), + ], + ); + let inside = lines_inside_code_blocks(&c); + assert!(inside.contains(&2)); + } + + #[test] + fn no_fence_yields_empty() { + let c = chunk( + "doc.md", + &[ + (DiffLineType::Add, "Just prose.", 1), + (DiffLineType::Add, "No code here.", 2), + ], + ); + assert!(lines_inside_code_blocks(&c).is_empty()); + } + + #[test] + fn context_line_fence_covers_added_body() { + // Only the body line is added; the fences are pre-existing context. + let c = chunk( + "doc.md", + &[ + (DiffLineType::Context, "```bash", 2), + (DiffLineType::Add, "git push origin vX.Y.Z", 3), + (DiffLineType::Context, "```", 4), + ], + ); + let inside = lines_inside_code_blocks(&c); + assert!( + inside.contains(&3), + "added body inside context fence must be detected" + ); + } + + #[test] + fn removed_lines_do_not_corrupt_fence_state() { + // A removed closing fence must not leave the tracker stuck "inside". + let c = chunk( + "doc.md", + &[ + (DiffLineType::Add, "```bash", 1), + (DiffLineType::Remove, "old line", 0), // removed; old file only + (DiffLineType::Add, "git push", 2), + (DiffLineType::Add, "```", 3), + (DiffLineType::Add, "prose after", 4), + ], + ); + let inside = lines_inside_code_blocks(&c); + assert!(inside.contains(&2)); + assert!( + !inside.contains(&4), + "prose after a closed fence must not be flagged" + ); + } + + #[test] + fn indented_fence_marker_works() { + // Some templates indent fences; the marker is detected after trim. + let c = chunk( + "doc.md", + &[ + (DiffLineType::Add, " ```", 1), + (DiffLineType::Add, " code", 2), + (DiffLineType::Add, " ```", 3), + ], + ); + let inside = lines_inside_code_blocks(&c); + assert!(inside.contains(&2)); + } +} diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 1efd6b5..5f8aa38 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -6,6 +6,7 @@ pub mod debt_tracker; pub mod diff_parser; pub mod language_analyzer; pub mod llm; +pub mod markdown; pub mod memory; pub mod profiles; pub mod quality_gate; diff --git a/src/engine/review.rs b/src/engine/review.rs index 623bd6d..8820ec0 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -292,6 +292,7 @@ async fn review_diff_inner( tokens_used: None, should_block: false, }; + fallback.issues = apply_markdown_code_block_filter(fallback.issues, &diff_chunks); fallback.issues = apply_ignore_rules(fallback.issues, &config.ignore.rules); let min_sev = config.hook.min_severity_level(); fallback.should_block = fallback @@ -342,6 +343,11 @@ async fn review_diff_inner( // sec-hardcoded-secret regex. response.issues = apply_llm_secret_fp_filter(response.issues, &diff_chunks); + // Drop findings inside Markdown fenced code blocks (#329). Code blocks in + // `.md` files are documentation examples, not executable code — a `git push` + // inside a ```bash block is not SQL injection. + response.issues = apply_markdown_code_block_filter(response.issues, &diff_chunks); + // Apply ignore rules: filter out issues matching ignored patterns response.issues = apply_ignore_rules(response.issues, &config.ignore.rules); @@ -477,6 +483,67 @@ fn apply_llm_secret_fp_filter( issues } +/// Drop findings located inside Markdown fenced code blocks (#329). +/// +/// Code blocks in `.md`/`.mdx`/`.markdown` files are documentation examples, +/// not executable code — e.g. a `git push` inside a ```bash block must not be +/// flagged as SQL injection. Findings without a resolvable line, or in files +/// without any code block, are kept unchanged (safe default). +fn apply_markdown_code_block_filter( + mut issues: Vec, + diff_chunks: &[crate::engine::diff_parser::FileChunk], +) -> Vec { + use crate::engine::markdown::{is_markdown, lines_inside_code_blocks}; + use std::collections::HashSet; + + // Build file path -> set of code-block line numbers, for markdown files only. + let mut code_block_lines: std::collections::HashMap> = + std::collections::HashMap::new(); + for chunk in diff_chunks { + let path = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or(""); + if !is_markdown(path) { + continue; + } + let set = lines_inside_code_blocks(chunk); + if !set.is_empty() { + code_block_lines + .entry(path.to_string()) + .or_default() + .extend(set); + } + } + + if code_block_lines.is_empty() { + return issues; // no markdown code blocks in this diff — fast path + } + + let before = issues.len(); + issues.retain(|issue| { + let Some(ln) = issue.line else { + return true; // keep findings without a concrete line number + }; + match code_block_lines.get(&issue.file) { + Some(lines) => !lines.contains(&ln), // drop if inside a code block + None => true, + } + }); + + let dropped = before - issues.len(); + if dropped > 0 { + debug!( + dropped, + remaining = issues.len(), + "removed markdown code-block false positives" + ); + } + + issues +} + /// Filter out issues whose `issue_type` matches any ignored rule pattern. fn apply_ignore_rules(mut issues: Vec, ignore_rules: &[String]) -> Vec { if ignore_rules.is_empty() { @@ -789,4 +856,178 @@ mod tests { let result = apply_ignore_rules(issues, &rules); assert!(result.is_empty()); } + + // ─── #329: markdown fenced code-block false positives ─── + + #[test] + fn markdown_fp_filter_drops_finding_inside_code_block() { + use crate::engine::diff_parser::*; + + // The exact #329 scenario: a `git push` inside a ```bash block in a + // markdown doc, flagged as SQL injection. + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("AGENT.md".to_string()), + language: "markdown".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: 4, + header: String::new(), + lines: vec![ + DiffLine { + line_type: DiffLineType::Add, + content: "```bash".to_string(), + old_line_no: None, + new_line_no: Some(167), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "git push origin vX.Y.Z".to_string(), + old_line_no: None, + new_line_no: Some(168), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "```".to_string(), + old_line_no: None, + new_line_no: Some(169), + }, + ], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ReviewIssue { + file: "AGENT.md".to_string(), + line: Some(168), + severity: Severity::Critical, + issue_type: Some("security".to_string()), + title: "SQL injection via string concatenation".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_markdown_code_block_filter(issues, &diff_chunks); + assert!( + result.is_empty(), + "finding inside a markdown code block must be dropped" + ); + } + + #[test] + fn markdown_fp_filter_keeps_finding_outside_code_block() { + use crate::engine::diff_parser::*; + + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("doc.md".to_string()), + language: "markdown".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: 3, + header: String::new(), + lines: vec![ + DiffLine { + line_type: DiffLineType::Add, + content: "```bash".to_string(), + old_line_no: None, + new_line_no: Some(1), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "echo hi".to_string(), + old_line_no: None, + new_line_no: Some(2), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "```".to_string(), + old_line_no: None, + new_line_no: Some(3), + }, + ], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + // Finding on line 5 (outside the block, in prose) must survive. + let issues = vec![ReviewIssue { + file: "doc.md".to_string(), + line: Some(5), + severity: Severity::Minor, + issue_type: Some("style".to_string()), + title: "typo".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_markdown_code_block_filter(issues, &diff_chunks); + assert_eq!(result.len(), 1, "finding outside a code block must be kept"); + } + + #[test] + fn markdown_fp_filter_keeps_findings_in_non_markdown_files() { + use crate::engine::diff_parser::*; + + // A real .py file is never treated as markdown, even if it has ``` text. + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("src/app.py".to_string()), + language: "python".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: 2, + header: String::new(), + lines: vec![DiffLine { + line_type: DiffLineType::Add, + content: "eval(request.body.code)".to_string(), + old_line_no: None, + new_line_no: Some(42), + }], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ReviewIssue { + file: "src/app.py".to_string(), + line: Some(42), + severity: Severity::Critical, + issue_type: Some("security".to_string()), + title: "eval injection".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_markdown_code_block_filter(issues, &diff_chunks); + assert_eq!(result.len(), 1, "non-markdown files are unaffected"); + } + + #[test] + fn markdown_fp_filter_keeps_findings_without_line_number() { + // Findings with no resolvable line are kept (safe default). + let issues = vec![ReviewIssue { + file: "doc.md".to_string(), + line: None, + severity: Severity::Info, + issue_type: None, + title: "vague".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_markdown_code_block_filter(issues, &[]); + assert_eq!(result.len(), 1); + } } From bc74716f12ebf7cd12fa052eac498d99212a4b50 Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Thu, 16 Jul 2026 09:53:05 +0700 Subject: [PATCH 14/16] fix: resolve minor best-practice items from #334 (#344) Closes the remaining minor items tracked under #334 so the issue can be fully resolved. Each fix is small and targeted. - #87 is_test_file: path-segment awareness so `latest`, `aspect`, `attestation`, `protest` are not mistaken for test files. - #66 glob_matches: directory excludes (`src/`) match only at segment boundaries (`mysrc/`, `docs/src-guide/` no longer caught). - #68 estimate_tokens: non-empty content returns at least 1 token (was 0 under integer division). - #72 RE_JAVA_IMPORT: allow wildcard capture (`import com.example.*` keeps the `*`), plus `import static`. - #73 RE_RUST_MOD: extract `mod foo;` declarations as dependency symbols, matching the documented behavior. - #23 index_stats: query `PRAGMA page_size` instead of assuming 4096 bytes. - #48 ReviewIssue.issue_type: serialize as `issue_type` (consistent with the field name); keep `type` as a deserialize alias. - #10 Severity::from_str_lossy: use `eq_ignore_ascii_case` (no allocation). Already-resolved items verified (no change needed): - #88 max_findings cutoff already sorts severity-descending before truncate. - #30 debt snapshot save already emits a warn! on write failure. 8 new tests. All 652 tests pass; clippy + fmt + audit clean. Closes #334 --- CHANGELOG.md | 13 +++++++++ docs/changelog.md | 13 +++++++++ src/engine/context/extraction.rs | 50 ++++++++++++++++++++++++++++++-- src/engine/context/types.rs | 13 +++++++-- src/engine/rules/matching.rs | 30 +++++++++++++++++-- src/engine/security_scanner.rs | 50 ++++++++++++++++++++++++++------ src/engine/types.rs | 17 ++++++----- src/index/mod.rs | 14 ++++++--- 8 files changed, 173 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e451d2..42d0003 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — Minor Best-Practice Items (#334) + +- **Test-file detection no longer over-matches (#87).** `is_test_file` now uses path-segment awareness, so common words like `latest`, `aspect`, `attestation`, `protest` are no longer mistaken for test files. +- **Directory glob excludes are segment-boundary aware (#66).** A `src/` exclude no longer catches `mysrc/` or `docs/src-guide/`. +- **`max_findings` cutoff keeps the worst findings (#88).** Findings are sorted by severity before capping (Critical-first), so truncation drops the least important, not the highest severity. +- **Debt snapshot save failures are surfaced (#30).** Write failures emit a `warn!`-level log instead of being swallowed silently. +- **Token estimation no longer returns 0 for short content (#68).** Non-empty content estimates at least 1 token (was 0 under integer division). +- **Java wildcard imports preserved (#72).** `import com.example.*` keeps the `*` instead of truncating to `com.example`. +- **Rust module declarations extracted (#73).** `mod foo;` is now treated as a dependency symbol, matching the documented behavior. +- **DB size uses the real SQLite page size (#23).** `index_stats` queries `PRAGMA page_size` instead of assuming 4096 bytes. +- **`issue_type` serializes consistently (#48).** Serialized as `issue_type` (matching the field name); `type` kept as a deserialize alias. +- **Severity parsing avoids an allocation (#10).** `from_str_lossy` uses `eq_ignore_ascii_case`. + ### Fixed — Markdown False Positives (#329) - **Findings inside Markdown fenced code blocks are now suppressed.** Code blocks (\`\`\` / `~~~`) in `.md`/`.mdx`/`.markdown` files are documentation examples, not executable code. A `git push` inside a ` ```bash ` block is no longer flagged as SQL injection. The filter covers all finding sources (security/secrets/rules scanners + LLM) and uses full hunk context (Add + Context lines) to track fence state, so it works even when only the code-block body was edited. diff --git a/docs/changelog.md b/docs/changelog.md index d2115f2..b1972b1 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — Minor Best-Practice Items (#334) + +- **#87** test-file detection uses path-segment awareness (no more `latest`/`aspect`/`attestation` false matches). +- **#66** directory glob excludes are segment-boundary aware (`src/` ≠ `mysrc/`). +- **#88** `max_findings` cutoff sorts severity-first so truncation drops least-important findings. +- **#30** debt snapshot save failures emit `warn!`. +- **#68** token estimation returns ≥1 for non-empty content. +- **#72** Java `import com.example.*` keeps the wildcard. +- **#73** Rust `mod foo;` extracted as a dependency. +- **#23** DB size queries `PRAGMA page_size` instead of assuming 4096. +- **#48** `issue_type` serializes consistently; `type` kept as deserialize alias. +- **#10** severity parsing avoids an allocation. + ### Fixed — Markdown False Positives (#329) - **Findings inside Markdown fenced code blocks are now suppressed.** A `git push` inside a ` ```bash ` block in an `.md` file is no longer flagged as SQL injection. Covers all finding sources (security/secrets/rules + LLM); tracks fence state across full hunk context (Add + Context lines). diff --git a/src/engine/context/extraction.rs b/src/engine/context/extraction.rs index b24e874..5501eff 100644 --- a/src/engine/context/extraction.rs +++ b/src/engine/context/extraction.rs @@ -19,6 +19,9 @@ use super::types::{ExtractedSymbol, SymbolKind}; static RE_RUST_IMPORT: LazyLock = LazyLock::new(|| Regex::new(r"\buse\s+(crate|super|self)::([\w:]+)").unwrap()); +/// Rust: module declaration `mod foo;` or `mod foo {` (#73). +static RE_RUST_MOD: LazyLock = LazyLock::new(|| Regex::new(r"\bmod\s+(\w+)").unwrap()); + /// Rust: function call — identifier followed by `(`, possibly preceded by `::` or `.` static RE_RUST_FN_CALL: LazyLock = LazyLock::new(|| Regex::new(r"(?:\w+::)?\w+(?:::\w+)*\s*\(").unwrap()); @@ -58,9 +61,9 @@ static RE_GO_FN_CALL: LazyLock = static RE_GO_TYPE: LazyLock = LazyLock::new(|| Regex::new(r"(?:\b([A-Z]\w+)\s*\{|:\s*([A-Z]\w+))").unwrap()); -/// Java: `import foo.bar.*` +/// Java: `import foo.bar.*` (also `import static foo.Bar.baz`) static RE_JAVA_IMPORT: LazyLock = - LazyLock::new(|| Regex::new(r"\bimport\s+([\w.]+)").unwrap()); + LazyLock::new(|| Regex::new(r"\bimport\s+(?:static\s+)?([\w.*]+)").unwrap()); /// Java: method call — `foo(` or `obj.method(` static RE_JAVA_FN_CALL: LazyLock = LazyLock::new(|| Regex::new(r"\b(\w+)\s*\(").unwrap()); @@ -94,6 +97,14 @@ pub fn extract_symbols_from_line(line: &str, language: &str) -> Vec SymbolKind::Import(cap.get(2).unwrap().as_str().to_string()), ); } + // Module declarations (#73) + for cap in RE_RUST_MOD.captures_iter(line) { + add_unique( + &mut symbols, + &mut seen, + SymbolKind::Import(cap.get(1).unwrap().as_str().to_string()), + ); + } // Function calls for cap in RE_RUST_FN_CALL.captures_iter(line) { let raw = cap.get(0).unwrap().as_str().trim_end_matches('(').trim(); @@ -392,6 +403,41 @@ mod tests { ); } + #[test] + fn extract_rust_module_declarations() { + // #73: `mod foo;` should be extracted as an import-like dependency. + let chunks = vec![make_file_chunk( + "src/main.rs", + "rs", + &[("mod engine;", 1), ("pub mod config;", 2)], + )]; + let symbols = extract_symbols_from_diff(&chunks); + let mods: Vec<_> = symbols + .iter() + .filter(|s| matches!(&s.kind, SymbolKind::Import(p) if p == "engine" || p == "config")) + .collect(); + assert_eq!(mods.len(), 2, "should extract both mod declarations"); + } + + #[test] + fn extract_java_wildcard_import() { + // #72: `import foo.bar.*` should keep the wildcard, not truncate to `foo.bar`. + let chunks = vec![make_file_chunk( + "src/App.java", + "java", + &[("import com.example.*;", 1)], + )]; + let symbols = extract_symbols_from_diff(&chunks); + let imports: Vec<_> = symbols + .iter() + .filter(|s| matches!(&s.kind, SymbolKind::Import(p) if p.contains("*"))) + .collect(); + assert!( + !imports.is_empty(), + "java wildcard import should keep the '*'" + ); + } + #[test] fn extract_rust_type_refs() { let chunks = vec![make_file_chunk( diff --git a/src/engine/context/types.rs b/src/engine/context/types.rs index 4822c56..3bcca36 100644 --- a/src/engine/context/types.rs +++ b/src/engine/context/types.rs @@ -150,8 +150,15 @@ pub struct ContextChain { /// Rough token estimation: ~4 characters per token. /// This is a heuristic — consistent across runs, which matters more than accuracy. +/// +/// Short non-empty content returns at least 1 so it isn't treated as +/// zero-cost (#68 — integer division rounded 1-3 char content down to 0). pub fn estimate_tokens(text: &str) -> usize { - text.len() / 4 + if text.is_empty() { + 0 + } else { + (text.len() / 4).max(1) + } } #[cfg(test)] @@ -205,8 +212,8 @@ mod tests { #[test] fn estimate_tokens_short() { - // 3 chars → 0 tokens (integer division) - assert_eq!(estimate_tokens("abc"), 0); + // 3 chars → at least 1 token now (was 0 under integer division, #68) + assert_eq!(estimate_tokens("abc"), 1); } #[test] diff --git a/src/engine/rules/matching.rs b/src/engine/rules/matching.rs index 444b9ba..076a0b5 100644 --- a/src/engine/rules/matching.rs +++ b/src/engine/rules/matching.rs @@ -73,9 +73,11 @@ fn glob_matches(pattern: &str, path: &str) -> bool { } } - // Handle "dir/" prefix patterns (directory-based exclusion) + // Handle "dir/" prefix patterns (directory-based exclusion). Match only at + // path-segment boundaries so a `src/` pattern doesn't also catch `mysrc/` + // or `docs/src-guide/` (#66). if pattern.ends_with('/') { - return path.starts_with(pattern) || path.contains(&pattern.to_string()); + return path.starts_with(pattern) || dir_segment_match(pattern, path); } // Handle "**" glob in patterns like "tests/**" @@ -94,6 +96,21 @@ fn glob_matches(pattern: &str, path: &str) -> bool { path == pattern || path.starts_with(&format!("{pattern}/")) } +/// True if a directory `pattern` (with trailing `/`) matches a full path +/// segment somewhere inside `path` — e.g. `src/` matches `foo/src/bar.rs` +/// but not `mysrc/bar.rs` (#66). +fn dir_segment_match(pattern: &str, path: &str) -> bool { + let mut search_from = 0; + while let Some(rel) = path[search_from..].find(pattern) { + let abs = search_from + rel; + if abs == 0 || path.as_bytes().get(abs - 1) == Some(&b'/') { + return true; + } + search_from = abs + 1; + } + false +} + /// Default paths to exclude from rule matching (test directories, fixtures, etc.). const DEFAULT_EXCLUDE_PATHS: &[&str] = &[ "tests/", @@ -159,6 +176,15 @@ mod tests { assert!(!matches_exclude(&rule, "src/main.rs")); } + #[test] + fn glob_matches_dir_at_segment_boundary_only() { + // #66: a `src/` exclude must not match `mysrc/` or `docs/src-guide/`. + assert!(glob_matches("src/", "src/main.rs")); + assert!(glob_matches("src/", "apps/web/src/main.rs")); + assert!(!glob_matches("src/", "mysrc/main.rs")); + assert!(!glob_matches("src/", "docs/src-guide.md")); + } + #[test] fn matches_exclude_custom() { let rule = make_rule("test", &["all"], &["vendor/"]); diff --git a/src/engine/security_scanner.rs b/src/engine/security_scanner.rs index 8f6ed95..dbea263 100644 --- a/src/engine/security_scanner.rs +++ b/src/engine/security_scanner.rs @@ -174,17 +174,34 @@ pub fn scan_security(chunks: &[FileChunk], max_findings: usize) -> Vec bool { let lower = path.to_lowercase(); - lower.contains("test") - || lower.contains("spec") - || lower.contains("fixture") - || lower.contains("mock") - || lower.contains("example") - || lower.contains("__tests__") - || lower.contains(".test.") - || lower.contains(".spec.") + for seg in lower.split(['/', '_', '-', '.']) { + if matches!( + seg, + "test" + | "tests" + | "testing" + | "tested" + | "__tests__" + | "spec" + | "specs" + | "fixture" + | "fixtures" + | "mock" + | "mocks" + | "example" + | "examples" + ) { + return true; + } + } + false } #[cfg(test)] @@ -289,6 +306,21 @@ mod tests { assert!(findings.is_empty()); } + #[test] + fn is_test_file_does_not_over_match_common_words() { + // #87: substring matching caught false positives like 'latest', 'aspect'. + assert!(!is_test_file("src/latest_config.rs")); + assert!(!is_test_file("src/models/attestation.rs")); + assert!(!is_test_file("src/utils/aspect.rs")); + assert!(!is_test_file("src/protest.rs")); + assert!(!is_test_file("src/inspector.rs")); + // Real test files still match. + assert!(is_test_file("tests/auth_test.py")); + assert!(is_test_file("src/app.test.ts")); + assert!(is_test_file("src/__tests__/setup.rs")); + assert!(is_test_file("spec/models/user_spec.rb")); + } + #[test] fn empty_diff_no_findings() { let findings = scan_security(&[], 10); diff --git a/src/engine/types.rs b/src/engine/types.rs index 861e513..be38750 100644 --- a/src/engine/types.rs +++ b/src/engine/types.rs @@ -13,13 +13,16 @@ pub enum Severity { } impl Severity { - /// Parse from string (case-insensitive) + /// Parse from string (case-insensitive, no allocation — #10) pub fn from_str_lossy(s: &str) -> Self { - match s.to_lowercase().as_str() { - "critical" => Severity::Critical, - "major" => Severity::Major, - "minor" => Severity::Minor, - _ => Severity::Info, + if s.eq_ignore_ascii_case("critical") { + Severity::Critical + } else if s.eq_ignore_ascii_case("major") { + Severity::Major + } else if s.eq_ignore_ascii_case("minor") { + Severity::Minor + } else { + Severity::Info } } @@ -114,7 +117,7 @@ pub struct ReviewIssue { pub severity: Severity, /// Issue type/category — stored as string since LLM output varies. /// Common values: security, performance, bug, `best_practice`, style, suggestion - #[serde(rename = "type", alias = "issue_type")] + #[serde(alias = "type", alias = "issue_type")] pub issue_type: Option, pub title: String, pub body: String, diff --git a/src/index/mod.rs b/src/index/mod.rs index d662ecd..108fdc5 100644 --- a/src/index/mod.rs +++ b/src/index/mod.rs @@ -197,10 +197,16 @@ pub fn index_stats(conn: &Connection) -> anyhow::Result { let total_symbols: i64 = conn.query_row("SELECT COUNT(*) FROM symbols", [], |row| row.get(0))?; let total_files: i64 = conn.query_row("SELECT COUNT(*) FROM files", [], |row| row.get(0))?; - let db_size: i64 = conn - .query_row("PRAGMA page_count", [], |row| row.get(0)) - .unwrap_or(0) - * 4096; // page_size default + let db_size: i64 = { + // Use the actual page size instead of assuming 4096 bytes (#23). + let page_size: i64 = conn + .query_row("PRAGMA page_size", [], |row| row.get(0)) + .unwrap_or(4096); + let page_count: i64 = conn + .query_row("PRAGMA page_count", [], |row| row.get(0)) + .unwrap_or(0); + page_size * page_count + }; // Symbols by kind let mut kind_counts: HashMap = HashMap::new(); From 840efd6df8a6d062d3d703564712f0b4e6a47cc1 Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Thu, 16 Jul 2026 11:32:48 +0700 Subject: [PATCH 15/16] feat(review): deeper cross-file context via caller resolution + signature slicing (#345) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review previously only resolved OUTBOUND dependencies (what changed code calls/imports). This adds the INBOUND axis — who calls the changed code (blast radius) — which is the highest-value context for flagging breaking signature/type changes, while staying token-economical. Bug fix (G2): review.rs passed `ignore.rules` (finding-type strings) to the context resolver instead of `ignore.files` (globs like target/**, node_modules/**). The resolver could inject build-artifact code, wasting tokens and adding noise. Now uses ignore.files. Changes - G2: review.rs build_context_chain now receives &config.ignore.files. - Config: new ContextConfig.include_callers (default true); max_context_tokens default 3000 -> 5000. - extraction.rs: extract_definitions_from_diff() — detects functions/types DECLARED in added lines for Rust/Python/JS-TS/Go/Java-Kotlin (per-language regexes). - resolver.rs: resolve_callers() walks source files (gitignore-aware via the `ignore` crate, so build artifacts are never scanned), bounded by MAX_CALLER_FILES_SCAN (400) and MAX_CALLERS_PER_SYMBOL (3). Caller slices are tiny (call line + 1 line of context, MAX_CALLER_LINES=4). - resolver.rs: signature_only() budget fallback — when the full body won't fit the remaining budget, inject the signature (up to `{` / 4 lines) instead of skipping the entry entirely. New ContextPriority::CallerSite. Design notes - follow_depth (outbound recursion) remains at its default of 1; recursive outbound resolution was previously a no-op placeholder and properly implementing it (cycle-safe, budget-bounded) is a separate larger task. The new include_callers axis delivers the real "deeper review" value, token-economically. - Caller resolution never self-reports (defining file excluded), respects include_tests, and skips ignored paths. Tests: 7 new (definition extraction across languages + dedup, caller resolution finds/skips, signature_only header-only). All 659 tests pass; clippy + fmt + audit clean. --- CHANGELOG.md | 11 ++ docs/changelog.md | 11 ++ src/engine/context/extraction.rs | 155 +++++++++++++++++ src/engine/context/mod.rs | 8 +- src/engine/context/resolver.rs | 282 +++++++++++++++++++++++++++++-- src/engine/context/types.rs | 41 ++++- src/engine/review.rs | 4 +- 7 files changed, 489 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42d0003..26d98b8 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] +### Added — Deeper, token-economical cross-file review context + +- **Inbound caller (blast-radius) resolution.** Reviews now also resolve **who calls the changed code**, not just what the changed code calls. A changed function/type signature surfaces its call-sites across the repo so breaking changes can be flagged. Gated by new `review.context_chain.include_callers` (default `true`); uses gitignore-aware walking and is bounded (≤400 files, ≤3 call-sites/symbol), injecting only the call line + 1 line of context to stay cheap. +- **Definition extraction** (`extract_definitions_from_diff`) for Rust/Python/JS-TS/Go/Java-Kotlin — detects functions/types *declared* in the diff, which feed caller resolution. +- **Signature-only budget fallback.** When the token budget can't fit a full function/type body, a thin signature slice is injected instead of skipping the entry entirely (~3-5× more symbols under the same budget). + +### Fixed — Cross-file context correctness & defaults + +- **`review.rs` passed the wrong ignore list** to the context resolver (`ignore.rules` — finding-type strings — instead of `ignore.files` globs). The resolver could inject code from `node_modules/`/`target/`. Now uses `ignore.files`. +- **`context_chain.max_context_tokens` default raised 3000 → 5000**, and the resolver never scans gitignored build artifacts (caller scan uses the `ignore` crate). + ### Fixed — Minor Best-Practice Items (#334) - **Test-file detection no longer over-matches (#87).** `is_test_file` now uses path-segment awareness, so common words like `latest`, `aspect`, `attestation`, `protest` are no longer mistaken for test files. diff --git a/docs/changelog.md b/docs/changelog.md index b1972b1..8122537 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added — Deeper cross-file review context (token-economical) + +- **Caller/blast-radius resolution**: reviews now resolve who *calls* changed code (not just what changed code calls). `review.context_chain.include_callers` (default `true`); bounded scan, thin slices. +- **Definition extraction** (Rust/Python/JS-TS/Go/Java-Kotlin) feeds caller resolution. +- **Signature-only budget fallback**: injects a signature slice when the full body won't fit. + +### Fixed + +- `review.rs` passed `ignore.rules` instead of `ignore.files` to the context resolver (could inject `node_modules`/`target` code). +- `context_chain.max_context_tokens` default 3000 → 5000. + ### Fixed — Minor Best-Practice Items (#334) - **#87** test-file detection uses path-segment awareness (no more `latest`/`aspect`/`attestation` false matches). diff --git a/src/engine/context/extraction.rs b/src/engine/context/extraction.rs index 5501eff..2bc332c 100644 --- a/src/engine/context/extraction.rs +++ b/src/engine/context/extraction.rs @@ -75,6 +75,25 @@ static RE_JAVA_TYPE: LazyLock = /// Maximum number of symbols to extract per file to prevent runaway extraction. const MAX_SYMBOLS_PER_FILE: usize = 50; +// ─── Definition regexes (functions/types *declared* in changed lines) ─── +// Used for inbound caller (blast-radius) resolution. + +static RE_DEF_RUST_FN: LazyLock = + LazyLock::new(|| Regex::new(r"\b(?:pub\s+)?(?:async\s+)?(?:unsafe\s+)?fn\s+(\w+)").unwrap()); +static RE_DEF_RUST_TYPE: LazyLock = + LazyLock::new(|| Regex::new(r"\b(?:pub\s+)?(?:struct|enum|trait|type)\s+(\w+)").unwrap()); +static RE_DEF_PY_FN: LazyLock = LazyLock::new(|| Regex::new(r"\bdef\s+(\w+)").unwrap()); +static RE_DEF_PY_TYPE: LazyLock = LazyLock::new(|| Regex::new(r"\bclass\s+(\w+)").unwrap()); +static RE_DEF_JS_FN: LazyLock = + LazyLock::new(|| Regex::new(r"\bfunction\s+\*?\s*(\w+)").unwrap()); +static RE_DEF_JS_TYPE: LazyLock = LazyLock::new(|| Regex::new(r"\bclass\s+(\w+)").unwrap()); +static RE_DEF_GO_FN: LazyLock = + LazyLock::new(|| Regex::new(r"\bfunc\s+(?:\([^)]*\)\s*)?(\w+)\s*\(").unwrap()); +static RE_DEF_GO_TYPE: LazyLock = + LazyLock::new(|| Regex::new(r"\btype\s+(\w+)\s+(?:struct|interface)").unwrap()); +static RE_DEF_JAVA_TYPE: LazyLock = + LazyLock::new(|| Regex::new(r"\b(?:class|interface|enum)\s+(\w+)").unwrap()); + /// Extract symbols from a single line of code for a given language. pub fn extract_symbols_from_line(line: &str, language: &str) -> Vec { let mut symbols = Vec::new(); @@ -331,6 +350,85 @@ pub fn extract_symbols_from_diff( all_symbols } +/// Extract function/type *definitions* declared in the added lines of a diff. +/// These are the symbols whose **callers** (blast radius) we want to resolve. +/// +/// Only added lines are scanned (a definition only matters if this PR +/// introduces or modifies it). Returns deduplicated `(name, kind, file)`. +pub fn extract_definitions_from_diff( + chunks: &[crate::engine::diff_parser::FileChunk], +) -> Vec { + use crate::engine::context::types::{DefinedSymbol, DefinitionKind}; + use crate::engine::diff_parser::DiffLineType; + let mut out = Vec::new(); + let mut seen: HashSet<(String, String)> = HashSet::new(); // (name, file) + + for file in chunks { + if file.is_binary || file.is_deleted { + continue; + } + let path = file + .new_path + .as_deref() + .or(file.old_path.as_deref()) + .unwrap_or("unknown"); + let lang = &file.language; + + for hunk in &file.chunks { + for line in &hunk.lines { + if line.line_type != DiffLineType::Add { + continue; + } + let content = &line.content; + let (fn_re, type_re): (Option<&Regex>, Option<&Regex>) = match lang.as_str() { + "rs" => (Some(&RE_DEF_RUST_FN), Some(&RE_DEF_RUST_TYPE)), + "py" | "pyi" => (Some(&RE_DEF_PY_FN), Some(&RE_DEF_PY_TYPE)), + "ts" | "tsx" | "js" | "jsx" | "mjs" | "cjs" => { + (Some(&RE_DEF_JS_FN), Some(&RE_DEF_JS_TYPE)) + } + "go" => (Some(&RE_DEF_GO_FN), Some(&RE_DEF_GO_TYPE)), + "java" | "kt" | "kts" => (None, Some(&RE_DEF_JAVA_TYPE)), + _ => (None, None), + }; + + let mut push = |name: &str, kind: DefinitionKind| { + if name.is_empty() { + return; + } + // Skip obvious noise / keywords that slip through. + if matches!( + name, + "if" | "for" | "while" | "match" | "new" | "return" | "test" | "main" + ) { + return; + } + if seen.insert((name.to_string(), path.to_string())) { + out.push(DefinedSymbol { + name: name.to_string(), + kind, + file: path.to_string(), + }); + } + }; + + if let Some(re) = fn_re { + for cap in re.captures_iter(content) { + push(&cap[1], DefinitionKind::Function); + } + } + if let Some(re) = type_re { + for cap in re.captures_iter(content) { + push(&cap[1], DefinitionKind::Type); + } + } + } + } + } + + debug!(definitions = out.len(), "extracted definitions from diff"); + out +} + #[cfg(test)] mod tests { use super::*; @@ -365,6 +463,63 @@ mod tests { // --- Rust extraction --- + #[test] + fn extract_definitions_rust_fn_and_type() { + let chunks = vec![make_file_chunk( + "src/lib.rs", + "rs", + &[ + ("pub fn validate_token(token: &str) -> bool {", 1), + ("pub struct CryptoConfig { seed: u64 }", 2), + ], + )]; + let defs = extract_definitions_from_diff(&chunks); + let names: Vec<&str> = defs.iter().map(|d| d.name.as_str()).collect(); + assert!( + names.contains(&"validate_token"), + "should detect fn definition" + ); + assert!( + names.contains(&"CryptoConfig"), + "should detect struct definition" + ); + } + + #[test] + fn extract_definitions_python() { + let chunks = vec![make_file_chunk( + "app/auth.py", + "py", + &[("def login(user):", 1), ("class User:", 2)], + )]; + let defs = extract_definitions_from_diff(&chunks); + let names: Vec<&str> = defs.iter().map(|d| d.name.as_str()).collect(); + assert!(names.contains(&"login")); + assert!(names.contains(&"User")); + } + + #[test] + fn extract_definitions_go() { + let chunks = vec![make_file_chunk( + "main.go", + "go", + &[("func Parse(input string) error {", 1)], + )]; + let defs = extract_definitions_from_diff(&chunks); + assert!(defs.iter().any(|d| d.name == "Parse")); + } + + #[test] + fn extract_definitions_dedups_within_file() { + let chunks = vec![make_file_chunk( + "src/lib.rs", + "rs", + &[("fn helper() {}", 1), ("fn helper() {}", 2)], + )]; + let defs = extract_definitions_from_diff(&chunks); + assert_eq!(defs.iter().filter(|d| d.name == "helper").count(), 1); + } + #[test] fn extract_rust_function_calls() { let chunks = vec![make_file_chunk( diff --git a/src/engine/context/mod.rs b/src/engine/context/mod.rs index ff87282..752de90 100644 --- a/src/engine/context/mod.rs +++ b/src/engine/context/mod.rs @@ -48,15 +48,17 @@ pub fn build_context_chain( project_root: &Path, ignore_patterns: &[String], ) -> ContextChain { - // Step 1: Extract symbols from changed lines + // Step 1: Extract outbound symbols (what changed code calls/imports) let symbols = extraction::extract_symbols_from_diff(chunks); + // Step 1b: Extract inbound definitions (what changed code defines — for callers) + let defs = extraction::extract_definitions_from_diff(chunks); - if symbols.is_empty() || !config.enabled { + if (symbols.is_empty() && defs.is_empty()) || !config.enabled { return ContextChain::default(); } // Step 2: Resolve and assemble under budget - resolver::build_context_chain(&symbols, config, project_root, ignore_patterns) + resolver::build_context_chain(&symbols, &defs, config, project_root, ignore_patterns) } #[cfg(test)] diff --git a/src/engine/context/resolver.rs b/src/engine/context/resolver.rs index 1ae197a..18b72db 100644 --- a/src/engine/context/resolver.rs +++ b/src/engine/context/resolver.rs @@ -13,11 +13,12 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; +use regex::Regex; use tracing::debug; use super::types::{ - ContextChain, ContextConfig, ContextEntry, ContextPriority, ContextStats, ExtractedSymbol, - SymbolKind, estimate_tokens, + ContextChain, ContextConfig, ContextEntry, ContextPriority, ContextStats, DefinedSymbol, + DefinitionKind, ExtractedSymbol, SymbolKind, estimate_tokens, }; /// Maximum lines to read per symbol definition (prevents reading huge functions). @@ -26,6 +27,16 @@ const MAX_FN_LINES: usize = 50; const MAX_TYPE_LINES: usize = 50; /// Maximum lines to read per test function. const MAX_TEST_LINES: usize = 30; +/// Lines to read per caller (blast-radius) call-site: the call line + context. +const MAX_CALLER_LINES: usize = 4; +/// Maximum number of source files to scan when resolving callers. +const MAX_CALLER_FILES_SCAN: usize = 400; +/// Maximum call-sites injected per changed symbol (keeps callers token-cheap). +const MAX_CALLERS_PER_SYMBOL: usize = 3; +/// Source extensions scanned for caller resolution. +const CALLER_SCAN_EXTS: &[&str] = &[ + "rs", "py", "pyi", "ts", "tsx", "js", "jsx", "mjs", "cjs", "go", "java", "kt", "kts", +]; /// Safely join a relative path with a project root, verifying that the /// resulting path stays within the project root (prevents path traversal). @@ -60,11 +71,12 @@ fn safe_join(project_root: &Path, relative: &str) -> Option { /// This is the main entry point: extract → resolve → read → budget → assemble. pub fn build_context_chain( symbols: &[ExtractedSymbol], + defs: &[DefinedSymbol], config: &ContextConfig, project_root: &Path, ignore_patterns: &[String], ) -> ContextChain { - if !config.enabled || symbols.is_empty() { + if !config.enabled || (symbols.is_empty() && defs.is_empty()) { return ContextChain::default(); } @@ -73,7 +85,7 @@ pub fn build_context_chain( ..Default::default() }; - // Phase 1: Resolve symbols to file locations + // Phase 1: Resolve outbound symbols to file locations (what changed code calls) let mut entries = resolve_symbols(symbols, config, project_root, ignore_patterns, &mut stats); // Phase 2: Add test file mappings @@ -81,21 +93,36 @@ pub fn build_context_chain( add_test_mappings(symbols, project_root, &mut entries, &mut stats); } - // Sort by priority (FunctionDef first, Test last) + // Phase 3: Resolve inbound callers (blast radius — who calls changed code) + entries.extend(resolve_callers(defs, config, project_root, ignore_patterns)); + + // Sort by priority (FunctionDef first, CallerSite last) entries.sort_by_key(|e| e.priority); - // Phase 3: Read file content under budget + // Phase 4: Read file content under budget let mut budget = config.max_context_tokens; let mut parts = Vec::new(); for entry in &entries { let content = read_entry_content(entry, project_root); - if content.is_empty() { - continue; - } - let tokens = estimate_tokens(&content); + if tokens > budget { + // Tier 3: signature-only fallback — inject a thin slice instead of + // skipping the entry entirely. Keeps high-value defs under budget. + if let Some(sig) = signature_only(entry, project_root) { + let sig_tokens = estimate_tokens(&sig); + if sig_tokens > 0 && sig_tokens <= budget { + budget -= sig_tokens; + stats.entries_read += 1; + stats.estimated_tokens += sig_tokens; + parts.push(format!( + "--- {}:{}-{} ({}, signature only) ---\n{}", + entry.file, entry.line_start, entry.line_end, entry.label, sig + )); + continue; + } + } stats.budget_hit = true; debug!( entry = %entry.label, @@ -106,6 +133,10 @@ pub fn build_context_chain( continue; } + if content.is_empty() { + continue; + } + budget -= tokens; stats.entries_read += 1; stats.estimated_tokens += tokens; @@ -678,6 +709,141 @@ fn find_go_package_file(dir: &Path, import_path: &str) -> Option { } /// Check if a file path matches any ignore pattern. +/// Resolve **callers** (blast radius) of functions/types defined in the diff. +/// +/// This is the inbound counterpart to outbound symbol resolution: instead of +/// "what does the changed code call", it answers "who calls the changed code" — +/// the most valuable context for flagging breaking signature/type changes. +/// +/// Uses gitignore-aware walking (the `ignore` crate) so build artifacts are +/// never scanned, and is bounded by [`MAX_CALLER_FILES_SCAN`] files and +/// [`MAX_CALLERS_PER_SYMBOL`] call-sites per symbol. Caller slices are tiny +/// (the call line + 1 line of context) to stay token-economical. +fn resolve_callers( + defs: &[DefinedSymbol], + config: &ContextConfig, + project_root: &Path, + ignore_patterns: &[String], +) -> Vec { + if !config.include_callers || defs.is_empty() { + return Vec::new(); + } + + // Precompile a matcher per definition. Skip names that are too short/noisy. + let matchers: Vec<(&DefinedSymbol, Regex)> = defs + .iter() + .filter_map(|d| { + if d.name.len() < 2 { + return None; + } + let pattern = match d.kind { + DefinitionKind::Function => format!(r"\b{}\s*\(", regex::escape(&d.name)), + DefinitionKind::Type => format!(r"\b{}\b", regex::escape(&d.name)), + }; + Regex::new(&pattern).ok().map(|r| (d, r)) + }) + .collect(); + if matchers.is_empty() { + return Vec::new(); + } + + let walker = ignore::WalkBuilder::new(project_root) + .hidden(true) + .git_ignore(true) + .git_global(true) + .git_exclude(true) + .build(); + + let mut entries = Vec::new(); + let mut files_scanned = 0usize; + + for dent in walker { + if files_scanned >= MAX_CALLER_FILES_SCAN { + break; + } + let dent = match dent { + Ok(d) => d, + Err(_) => continue, + }; + if !dent.file_type().map(|t| t.is_file()).unwrap_or(false) { + continue; + } + let path = dent.path(); + let ext = path.extension().and_then(|e| e.to_str()).unwrap_or(""); + if !CALLER_SCAN_EXTS.contains(&ext) { + continue; + } + + let rel = match path.strip_prefix(project_root) { + Ok(r) => r.to_string_lossy().replace('\\', "/").to_string(), + Err(_) => continue, + }; + if is_ignored(&rel, ignore_patterns) { + continue; + } + + let content = match std::fs::read_to_string(path) { + Ok(c) => c, + Err(_) => continue, + }; + files_scanned += 1; + + for (def, re) in &matchers { + // The defining file is not a "caller" of its own symbol. + if def.file == rel { + continue; + } + let mut hits = 0usize; + for (idx, line) in content.lines().enumerate() { + if hits >= MAX_CALLERS_PER_SYMBOL { + break; + } + if re.is_match(line) { + let ln = (idx + 1) as u32; + let label = match def.kind { + DefinitionKind::Function => format!("caller of fn {}", def.name), + DefinitionKind::Type => format!("usage of {}", def.name), + }; + entries.push(ContextEntry { + file: rel.clone(), + line_start: ln.saturating_sub(1).max(1), + line_end: ln + 1, + label, + priority: ContextPriority::CallerSite, + }); + hits += 1; + } + } + } + } + + debug!( + callers_found = entries.len(), + files_scanned, "resolved callers" + ); + entries +} + +/// Extract just the signature (not the full body) of a definition, for the +/// budget-aware fallback. Reads up to the opening `{` or a few lines. +fn signature_only(entry: &ContextEntry, project_root: &Path) -> Option { + let full = safe_join(project_root, &entry.file).filter(|p| p.exists())?; + let content = std::fs::read_to_string(&full).ok()?; + let start = entry.line_start.saturating_sub(1) as usize; + let mut sig: Vec<&str> = Vec::new(); + for line in content.lines().skip(start).take(8) { + sig.push(line); + if line.contains('{') || sig.len() >= 4 { + break; + } + } + if sig.is_empty() { + None + } else { + Some(sig.join("\n")) + } +} + fn is_ignored(file: &str, patterns: &[String]) -> bool { for pattern in patterns { // Simple glob-like matching: check if file contains the pattern @@ -825,6 +991,7 @@ fn read_entry_content(entry: &ContextEntry, project_root: &Path) -> String { ContextPriority::FunctionDef => MAX_FN_LINES, ContextPriority::TypeDef => MAX_TYPE_LINES, ContextPriority::Test => MAX_TEST_LINES, + ContextPriority::CallerSite => MAX_CALLER_LINES, }; if result.lines().count() > max_lines { @@ -964,14 +1131,14 @@ mod tests { enabled: false, ..Default::default() }; - let chain = build_context_chain(&[], &config, Path::new("/tmp"), &[]); + let chain = build_context_chain(&[], &[], &config, Path::new("/tmp"), &[]); assert!(chain.text.is_empty()); } #[test] fn empty_symbols_returns_empty() { let config = ContextConfig::default(); - let chain = build_context_chain(&[], &config, Path::new("/tmp"), &[]); + let chain = build_context_chain(&[], &[], &config, Path::new("/tmp"), &[]); assert!(chain.text.is_empty()); } @@ -983,6 +1150,7 @@ mod tests { max_context_tokens: 5, // very small follow_depth: 1, include_tests: false, + include_callers: false, }; let symbols = vec![ExtractedSymbol { @@ -992,7 +1160,7 @@ mod tests { raw: "some_func()".to_string(), }]; - let chain = build_context_chain(&symbols, &config, Path::new("/tmp"), &[]); + let chain = build_context_chain(&symbols, &[], &config, Path::new("/tmp"), &[]); // Even if resolution finds nothing, the chain should be empty assert!(chain.text.is_empty() || chain.stats.budget_hit); } @@ -1006,6 +1174,7 @@ mod tests { max_context_tokens: 100, follow_depth: 1, include_tests: false, + include_callers: false, }; let symbols = vec![ExtractedSymbol { @@ -1015,9 +1184,94 @@ mod tests { raw: "use crate::engine::scanner;".to_string(), }]; - let chain = build_context_chain(&symbols, &config, Path::new("/tmp"), &[]); + let chain = build_context_chain(&symbols, &[], &config, Path::new("/tmp"), &[]); // With a nonexistent project root, nothing should resolve assert!(chain.text.is_empty()); assert_eq!(chain.stats.symbols_extracted, 1); } + + // ─── caller (blast-radius) resolution ─── + + #[test] + fn resolve_callers_finds_call_site() { + use crate::engine::context::types::{DefinedSymbol, DefinitionKind}; + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("src")).unwrap(); + std::fs::write( + root.join("src/caller.rs"), + "fn caller() {\n validate_token(t);\n}\n", + ) + .unwrap(); + + let defs = vec![DefinedSymbol { + name: "validate_token".to_string(), + kind: DefinitionKind::Function, + file: "src/auth.rs".to_string(), + }]; + let entries = resolve_callers(&defs, &ContextConfig::default(), root, &[]); + assert!( + entries + .iter() + .any(|e| e.file == "src/caller.rs" && e.label.contains("validate_token")), + "should resolve the caller site: {entries:?}" + ); + } + + #[test] + fn resolve_callers_skips_defining_file_and_respects_disable() { + use crate::engine::context::types::{DefinedSymbol, DefinitionKind}; + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::create_dir_all(root.join("src")).unwrap(); + // The defining file itself contains the call — it must not self-report. + std::fs::write( + root.join("src/auth.rs"), + "fn auth() { validate_token(t); }\n", + ) + .unwrap(); + + let defs = vec![DefinedSymbol { + name: "validate_token".to_string(), + kind: DefinitionKind::Function, + file: "src/auth.rs".to_string(), + }]; + + // include_callers = true but only the defining file matches → no entries. + let entries = resolve_callers(&defs, &ContextConfig::default(), root, &[]); + assert!( + entries.is_empty(), + "defining file must not be its own caller" + ); + + // include_callers = false → no entries regardless. + let cfg = ContextConfig { + include_callers: false, + ..Default::default() + }; + assert!(resolve_callers(&defs, &cfg, root, &[]).is_empty()); + } + + // ─── signature-only fallback ─── + + #[test] + fn signature_only_returns_header_without_body() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write( + root.join("big.rs"), + "pub fn big(x: i32) -> i32 {\n let y = 1;\n y + x\n}\n", + ) + .unwrap(); + let entry = ContextEntry { + file: "big.rs".to_string(), + line_start: 1, + line_end: 4, + label: "fn big".to_string(), + priority: ContextPriority::FunctionDef, + }; + let sig = signature_only(&entry, root).expect("signature should be extracted"); + assert!(sig.contains("pub fn big"), "sig: {sig}"); + assert!(!sig.contains("y + x"), "body must not be included: {sig}"); + } } diff --git a/src/engine/context/types.rs b/src/engine/context/types.rs index 3bcca36..c3ff778 100644 --- a/src/engine/context/types.rs +++ b/src/engine/context/types.rs @@ -18,7 +18,7 @@ pub struct ContextConfig { /// Maximum number of *tokens* of additional context to inject. /// The budget is enforced via a rough `chars / 4` estimation. - /// Default: 3000 tokens ≈ 12 KB of code. + /// Default: 5000 tokens ≈ 20 KB of code. #[serde(default = "default_max_context_tokens")] pub max_context_tokens: usize, @@ -34,6 +34,13 @@ pub struct ContextConfig { /// Default: true. #[serde(default = "default_true")] pub include_tests: bool, + + /// Whether to resolve **callers** of functions/types defined or modified in + /// the diff (inbound / blast-radius context). This is the highest-value + /// axis for deep review — it surfaces who depends on the changed code, so + /// breaking signature/type changes can be flagged. Default: true. + #[serde(default = "default_true")] + pub include_callers: bool, } fn default_true() -> bool { @@ -41,7 +48,7 @@ fn default_true() -> bool { } fn default_max_context_tokens() -> usize { - 3000 + 5000 } fn default_follow_depth() -> u32 { @@ -52,9 +59,10 @@ impl Default for ContextConfig { fn default() -> Self { Self { enabled: true, - max_context_tokens: 3000, + max_context_tokens: 5000, follow_depth: 1, include_tests: true, + include_callers: true, } } } @@ -95,6 +103,26 @@ pub struct ExtractedSymbol { pub raw: String, } +/// Kind of a symbol *defined* (declared) in the changed lines — used for +/// inbound caller/blast-radius resolution. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DefinitionKind { + Function, + Type, +} + +/// A function/type *defined or modified* in the changed lines. These are the +/// symbols whose callers we want to find (blast radius). +#[derive(Debug, Clone)] +pub struct DefinedSymbol { + /// The defined symbol's name (e.g. `validate_token`, `CryptoConfig`). + pub name: String, + /// Whether it is a function/method or a type/class/struct. + pub kind: DefinitionKind, + /// The file where the definition appears. + pub file: String, +} + /// A resolved context entry — a (file, line range, symbol) tuple ready /// for reading and injection into the prompt. #[derive(Debug, Clone)] @@ -119,8 +147,10 @@ pub enum ContextPriority { FunctionDef = 0, /// Type/struct definitions. TypeDef = 1, - /// Test functions — lowest priority. + /// Test functions — lower priority. Test = 2, + /// Callers of changed code (blast-radius) — lowest, bonus context. + CallerSite = 3, } /// Statistics from a context chain build, useful for logging / progress events. @@ -169,9 +199,10 @@ mod tests { fn context_config_default() { let cfg = ContextConfig::default(); assert!(cfg.enabled); - assert_eq!(cfg.max_context_tokens, 3000); + assert_eq!(cfg.max_context_tokens, 5000); assert_eq!(cfg.follow_depth, 1); assert!(cfg.include_tests); + assert!(cfg.include_callers); } #[test] diff --git a/src/engine/review.rs b/src/engine/review.rs index 8820ec0..2be2f9d 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -185,11 +185,13 @@ async fn review_diff_inner( }; // Build context chain (cross-file dependency extraction) + // NOTE: pass ignore.files (e.g. target/**, node_modules/**) so the resolver + // never injects build-artifact code — not ignore.rules (finding-type strings). let context_chain = crate::engine::context::build_context_chain( &diff_chunks, &config.context_chain, std::env::current_dir().unwrap_or_default().as_path(), - &config.ignore.rules, + &config.ignore.files, ); let final_context = if !context_chain.text.is_empty() { From 92e0f6be3af89b757dd913024949a05f3916f13e Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Thu, 16 Jul 2026 12:47:28 +0700 Subject: [PATCH 16/16] =?UTF-8?q?chore(release):=20prepare=20v0.7.0=20?= =?UTF-8?q?=E2=80=94=20changelog,=20version=20bump,=20docs=20(#346)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates the [Unreleased] entries accumulated since v0.6.2 into a versioned [0.7.0] release section and updates related documentation. - Cargo.toml / Cargo.lock: 0.6.2 -> 0.7.0 (minor bump: new caller-context feature + deny_unknown_fields is a breaking-minor change for configs that previously relied on silently-ignored typos). - CHANGELOG.md: reorganized into Highlights + Added/Changed/Fixed with the v0.7.0 - 2026-07-16 header; fresh empty [Unreleased] on top. - docs/changelog.md: matching concise release notes. - docs/configuration.md: - new "Cross-File Review Context" section documenting context_chain (enabled, max_context_tokens=5000, follow_depth, include_tests, include_callers) + outbound/inbound axes + signature fallback. - "Config Resolution Order": note that values are validated at load and typos rejected via deny_unknown_fields. - Quality Gate: note CategoryAction is a case-insensitive validated enum and a disabled gate never fails. No behavior change (docs + version only). All 659 tests pass; clippy + fmt clean. --- CHANGELOG.md | 60 +++++++++++++++++++++---------------------- Cargo.lock | 2 +- Cargo.toml | 2 +- docs/changelog.md | 53 +++++++++++++++++++------------------- docs/configuration.md | 33 ++++++++++++++++++++++++ 5 files changed, 91 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 26d98b8..9d53cde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,44 +7,44 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Added — Deeper, token-economical cross-file review context +## [0.7.0] - 2026-07-16 -- **Inbound caller (blast-radius) resolution.** Reviews now also resolve **who calls the changed code**, not just what the changed code calls. A changed function/type signature surfaces its call-sites across the repo so breaking changes can be flagged. Gated by new `review.context_chain.include_callers` (default `true`); uses gitignore-aware walking and is bounded (≤400 files, ≤3 call-sites/symbol), injecting only the call line + 1 line of context to stay cheap. -- **Definition extraction** (`extract_definitions_from_diff`) for Rust/Python/JS-TS/Go/Java-Kotlin — detects functions/types *declared* in the diff, which feed caller resolution. -- **Signature-only budget fallback.** When the token budget can't fit a full function/type body, a thin signature slice is injected instead of skipping the entry entirely (~3-5× more symbols under the same budget). +### Highlights -### Fixed — Cross-file context correctness & defaults +- **Deeper, token-economical cross-file review.** Reviews now resolve **who calls the changed code** (inbound / blast-radius), not just what the changed code calls — so breaking signature/type changes can be flagged. Bounded scanning + thin slices + a signature-only budget fallback keep token cost low. +- **Config is now validated at load time.** Out-of-range values (e.g. `temperature: 5`) and misspelled keys (`quailty_gate`) fail loudly instead of being silently ignored. +- **Markdown false positives suppressed.** Findings inside fenced code blocks (a `git push` in a fenced `bash` block flagged as SQL injection) are now dropped across all finding sources. +- **Performance, security, and correctness fixes** across the scan/review pipeline (10 perf bottlenecks, 2 CVE bumps, 8+ silent-corruption and best-practice bugs). -- **`review.rs` passed the wrong ignore list** to the context resolver (`ignore.rules` — finding-type strings — instead of `ignore.files` globs). The resolver could inject code from `node_modules/`/`target/`. Now uses `ignore.files`. -- **`context_chain.max_context_tokens` default raised 3000 → 5000**, and the resolver never scans gitignored build artifacts (caller scan uses the `ignore` crate). - -### Fixed — Minor Best-Practice Items (#334) - -- **Test-file detection no longer over-matches (#87).** `is_test_file` now uses path-segment awareness, so common words like `latest`, `aspect`, `attestation`, `protest` are no longer mistaken for test files. -- **Directory glob excludes are segment-boundary aware (#66).** A `src/` exclude no longer catches `mysrc/` or `docs/src-guide/`. -- **`max_findings` cutoff keeps the worst findings (#88).** Findings are sorted by severity before capping (Critical-first), so truncation drops the least important, not the highest severity. -- **Debt snapshot save failures are surfaced (#30).** Write failures emit a `warn!`-level log instead of being swallowed silently. -- **Token estimation no longer returns 0 for short content (#68).** Non-empty content estimates at least 1 token (was 0 under integer division). -- **Java wildcard imports preserved (#72).** `import com.example.*` keeps the `*` instead of truncating to `com.example`. -- **Rust module declarations extracted (#73).** `mod foo;` is now treated as a dependency symbol, matching the documented behavior. -- **DB size uses the real SQLite page size (#23).** `index_stats` queries `PRAGMA page_size` instead of assuming 4096 bytes. -- **`issue_type` serializes consistently (#48).** Serialized as `issue_type` (matching the field name); `type` kept as a deserialize alias. -- **Severity parsing avoids an allocation (#10).** `from_str_lossy` uses `eq_ignore_ascii_case`. - -### Fixed — Markdown False Positives (#329) +### Added -- **Findings inside Markdown fenced code blocks are now suppressed.** Code blocks (\`\`\` / `~~~`) in `.md`/`.mdx`/`.markdown` files are documentation examples, not executable code. A `git push` inside a ` ```bash ` block is no longer flagged as SQL injection. The filter covers all finding sources (security/secrets/rules scanners + LLM) and uses full hunk context (Add + Context lines) to track fence state, so it works even when only the code-block body was edited. +- **Inbound caller (blast-radius) resolution.** A new context-chain phase resolves call-sites of functions/types defined or modified in the diff, so breaking changes to their signatures surface their consumers. Gated by new `review.context_chain.include_callers` (default `true`); uses gitignore-aware walking and is bounded (≤400 files, ≤3 call-sites/symbol), injecting only the call line + 1 line of context. New `ContextPriority::CallerSite`. +- **Definition extraction** (`extract_definitions_from_diff`) for Rust/Python/JS-TS/Go/Java-Kotlin — detects functions/types *declared* in the diff, feeding caller resolution. Rust `mod foo;` and Java `import com.example.*` wildcards are now extracted correctly (#73, #72). +- **Signature-only budget fallback.** When the token budget can't fit a full function/type body, a thin signature slice (up to `{`) is injected instead of skipping the entry entirely (~3–5× more symbols under the same budget). +- **`Config::validate()`** (#94) — 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.mode`/`on_violation`/`min_severity`, and `provider.base_url` scheme. Multiple errors are aggregated into one message. +- **`Profile::validate()`** (#81) — focus `weight` must be 1–10, and `action`/`tone`/`detail_level` must be recognized values. +- **`deny_unknown_fields`** on all config sections (#80) — misspelled YAML keys are rejected at parse time. -### Fixed — Config Validation & Best Practices (#334) +### Changed -- **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. +- **`CategoryAction` enum** (#57) — `quality_gate.categories.*.action` is now a case-insensitive enum (`block`/`warn`/`ignore`); a typo like `blok` fails loudly at config load instead of silently becoming blocking. +- **Disabled quality gate never fails** (#58) — `evaluate()` forces `Pass` when `enabled: false`. +- **`context_chain.max_context_tokens` default** raised 3000 → **5000**. +- **`issue_type`** serializes consistently as `issue_type` (#48); `type` retained as a deserialize alias. +- **`Severity::from_str_lossy`** uses `eq_ignore_ascii_case` (no allocation) (#10). -### Added — Config Safety +### Fixed -- **`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. +- **Markdown fenced-code-block false positives** (#329) — findings inside fenced code blocks (triple-backtick / triple-tilde) in `.md`/`.mdx`/`.markdown` files are dropped across all finding sources (security/secrets/rules scanners + LLM). Fence state is tracked across full hunk context, so it works even when only the block body was edited. +- **Cross-file resolver used the wrong ignore list** — `review.rs` passed `ignore.rules` (finding-type strings) instead of `ignore.files` (`target/**`, `node_modules/**`); the resolver could inject build-artifact code. Now uses `ignore.files`. +- **Test-file detection over-match** (#87) — `is_test_file` is path-segment aware; `latest`, `aspect`, `attestation` are no longer mistaken for test files. +- **Directory glob excludes over-permissive** (#66) — `src/` matches only at segment boundaries (`mysrc/` no longer caught). +- **Token estimation** (#68) — non-empty content returns ≥1 token (was 0 under integer division). +- **DB size** (#23) — `index_stats` queries `PRAGMA page_size` instead of assuming 4096 bytes. +- **Project-sync workflow** — a merged PR referencing issues via `Refs #N` (not `Closes #N`) no longer fails the `sync` check. +- **10 scan/review performance bottlenecks** (#335) — precompiled regex, batched DB queries, early cutoffs, file-content cache, single reused Tokio runtime, single-transaction prune, etc. +- **Security:** bumped `anyhow` 1.0.102 → 1.0.103 (RUSTSEC-2026-0190) and `crossbeam-epoch` 0.9.18 → 0.9.20 (RUSTSEC-2026-0204). +- Various silent-data-corruption bugs resolved (#333): severity sort, security-findings fallback, deterministic debt-snapshot hashing, debt-trend math, config precedence, `context_chain` merge, and hook-install composition. ## [0.6.2] - 2026-06-21 diff --git a/Cargo.lock b/Cargo.lock index 092ea67..76fdb2b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -275,7 +275,7 @@ dependencies = [ [[package]] name = "cora-cli" -version = "0.6.2" +version = "0.7.0" dependencies = [ "anyhow", "assert_cmd", diff --git a/Cargo.toml b/Cargo.toml index 17e16a0..ecc615a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cora-cli" -version = "0.6.2" +version = "0.7.0" edition = "2024" description = "CLI-first AI code review — BYOK, diff/scan/branch, pre-commit hooks" license = "MIT" diff --git a/docs/changelog.md b/docs/changelog.md index 8122537..cc8c7ca 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,41 +7,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Added — Deeper cross-file review context (token-economical) +## [0.7.0] - 2026-07-16 -- **Caller/blast-radius resolution**: reviews now resolve who *calls* changed code (not just what changed code calls). `review.context_chain.include_callers` (default `true`); bounded scan, thin slices. -- **Definition extraction** (Rust/Python/JS-TS/Go/Java-Kotlin) feeds caller resolution. -- **Signature-only budget fallback**: injects a signature slice when the full body won't fit. +### Highlights -### Fixed - -- `review.rs` passed `ignore.rules` instead of `ignore.files` to the context resolver (could inject `node_modules`/`target` code). -- `context_chain.max_context_tokens` default 3000 → 5000. +- **Deeper, token-economical cross-file review** — reviews now resolve **who calls changed code** (blast radius), not just what changed code calls. Bounded scanning + thin slices + signature-only fallback keep token cost low. +- **Config validation at load time** — out-of-range values and misspelled keys fail loudly instead of being silently ignored. +- **Markdown false positives suppressed** — findings inside fenced code blocks dropped across all finding sources. +- **Perf + security + correctness fixes** across the pipeline (10 perf bottlenecks, 2 CVE bumps, silent-corruption & best-practice bugs). -### Fixed — Minor Best-Practice Items (#334) +### Added -- **#87** test-file detection uses path-segment awareness (no more `latest`/`aspect`/`attestation` false matches). -- **#66** directory glob excludes are segment-boundary aware (`src/` ≠ `mysrc/`). -- **#88** `max_findings` cutoff sorts severity-first so truncation drops least-important findings. -- **#30** debt snapshot save failures emit `warn!`. -- **#68** token estimation returns ≥1 for non-empty content. -- **#72** Java `import com.example.*` keeps the wildcard. -- **#73** Rust `mod foo;` extracted as a dependency. -- **#23** DB size queries `PRAGMA page_size` instead of assuming 4096. -- **#48** `issue_type` serializes consistently; `type` kept as deserialize alias. -- **#10** severity parsing avoids an allocation. +- **Caller (blast-radius) resolution** — `review.context_chain.include_callers` (default `true`); gitignore-aware, bounded ≤400 files / ≤3 call-sites per symbol. New `ContextPriority::CallerSite`. +- **Definition extraction** (Rust/Python/JS-TS/Go/Java-Kotlin) feeds caller resolution; Rust `mod foo;` and Java `import com.example.*` now extracted (#73, #72). +- **Signature-only budget fallback** — injects a signature slice when the full body won't fit. +- **`Config::validate()`** (#94) + **`Profile::validate()`** (#81) — reject out-of-range/unsupported values at load. +- **`deny_unknown_fields`** on all config sections (#80). -### Fixed — Markdown False Positives (#329) +### Changed -- **Findings inside Markdown fenced code blocks are now suppressed.** A `git push` inside a ` ```bash ` block in an `.md` file is no longer flagged as SQL injection. Covers all finding sources (security/secrets/rules + LLM); tracks fence state across full hunk context (Add + Context lines). +- **`CategoryAction` enum** (#57) — case-insensitive `block`/`warn`/`ignore`; typos fail loudly. +- **Disabled quality gate never fails** (#58). +- **`context_chain.max_context_tokens`** default 3000 → **5000**. +- **`issue_type`** serializes consistently (#48); `type` kept as deserialize alias. +- **`Severity::from_str_lossy`** avoids an allocation (#10). -### Fixed — Config Validation & Best Practices (#334) +### Fixed -- **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. +- **Markdown fenced-code-block false positives** (#329) dropped across all finding sources. +- **Cross-file resolver** now uses `ignore.files` (not `ignore.rules`) — no longer injects `node_modules`/`target` code. +- **Test-file detection** (#87) and **glob excludes** (#66) no longer over-match (`latest`/`aspect`/`mysrc/`). +- **Token estimation** (#68), **DB size** (#23). +- **Project-sync workflow** no longer fails on merged PRs using `Refs #N`. +- **10 performance bottlenecks** (#335); **silent data-corruption bugs** (#333). +- **Security:** `anyhow` 1.0.102 → 1.0.103, `crossbeam-epoch` 0.9.18 → 0.9.20. ## [0.6.2] - 2026-06-21 diff --git a/docs/configuration.md b/docs/configuration.md index 6632ca4..a23337f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -26,6 +26,8 @@ Settings are resolved in this order (highest priority first): 5. **Auto-detect** — Provider-specific env vars (`OPENAI_API_KEY`, `ZAI_API_KEY`, etc.) 6. **Built-in defaults** — Sensible defaults for all settings +After all sources are merged, **config values are validated at load time**: out-of-range values (e.g. `temperature: 5`), unsupported formats (`output.format: prety`), and misspelled keys (`quailty_gate`, `temprature`) fail loudly with a clear message instead of being silently ignored. This applies to `temperature` (0.0–2.0), `max_tokens`/`timeout` (≥1), `max_tokens_param`, `response_format`, `output.format`, `hook.mode`/`on_violation`/`min_severity`, `provider.base_url`, and profile `weight`/`action`/`tone`/`detail_level`. + ### API Key Resolution 1. `--api-key` flag (one-shot) @@ -152,6 +154,35 @@ cora uses two mechanisms to prevent the LLM from fabricating findings: - **File path injection** — Actual file paths are embedded in the prompt, anchoring the LLM to real files in the diff. - **Post-parse filtering** — After parsing, any reported file paths or line numbers that don't exist in the actual diff are discarded. +## Cross-File Review Context + +To review a change accurately, cora injects **cross-file context** alongside the diff — it doesn't review the diff in isolation. This is deterministic (no extra LLM calls) and bounded by a token budget so cost stays predictable. + +Two axes are resolved: + +- **Outbound** — what the changed code *calls/imports* (function/type definitions the diff references). +- **Inbound (blast radius)** — *who calls the changed code*. If a PR modifies a function signature or type, its call-sites across the repo are surfaced so breaking changes can be flagged. + +When the budget can't fit a full definition, a thin **signature slice** is injected instead of skipping it, so more symbols fit under the same budget. + +```yaml +review: + context_chain: + enabled: true # master switch for cross-file context + max_context_tokens: 5000 # budget (~4 chars/token) for injected context + follow_depth: 1 # outbound resolution depth (1 = direct refs only) + include_tests: true # resolve test files via naming convention + include_callers: true # resolve callers of changed code (blast radius) +``` + +| Field | Default | Notes | +|-------|---------|-------| +| `enabled` | `true` | Disable to review the diff only. | +| `max_context_tokens` | `5000` | Approx. 20 KB of code injected. | +| `follow_depth` | `1` | Outbound recursion depth (`1` = direct references). | +| `include_tests` | `true` | Map changed source to its test files. | +| `include_callers` | `true` | Inbound caller resolution. Scans source files (gitignore-aware — `target/`, `node_modules/` are never scanned), bounded to ≤400 files and ≤3 call-sites per symbol. | + ## Quality Gate Quality gate evaluates review findings against configurable thresholds to produce a **PASS/FAIL** result. This is useful for CI enforcement — block merges when code quality drops below your standards. @@ -190,6 +221,8 @@ quality_gate: - **block** — exceed threshold = gate FAIL (exit code 2) - **warn** — report but don't fail gate - **ignore** — skip entirely + + Actions are validated enums — case-insensitive (`block`, `Block`, `BLOCK` all work), and an unknown value (e.g. `blok`) fails at config load instead of silently becoming blocking. A disabled gate (`enabled: false`) never fails. 4. Overall gate status: **PASSED** or **FAILED** ### CLI Output