From af1e13654e0d0bb48e25cca8b490dc571572c93e Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Thu, 16 Jul 2026 09:46:24 +0700 Subject: [PATCH] fix: resolve minor best-practice items from #334 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();