fix: resolve minor best-practice items (#334)

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.

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).

src/engine/context/extraction.rs

@@ -19,6 +19,9 @@ use super::types::{ExtractedSymbol, SymbolKind};
 static RE_RUST_IMPORT: LazyLock<Regex> =
     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<Regex> = 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<Regex> =
     LazyLock::new(|| Regex::new(r"(?:\w+::)?\w+(?:::\w+)*\s*\(").unwrap());
@@ -58,9 +61,9 @@ static RE_GO_FN_CALL: LazyLock<Regex> =
 static RE_GO_TYPE: LazyLock<Regex> =
     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<Regex> =
-    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<Regex> = 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>
                     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(

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]

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/"]);

src/engine/security_scanner.rs

@@ -174,17 +174,34 @@ pub fn scan_security(chunks: &[FileChunk], max_findings: usize) -> Vec<RuleFindi
     findings
 }
 
-/// Check if a file path looks like a test/spec/fixture file.
+/// Check if a file path looks like a test/spec/fixture/mock/example file.
+///
+/// Uses path-segment awareness so common words like `latest`, `aspect`,
+/// `attestation`, `protest` are not mistaken for test files (#87). A segment
+/// is any run between `/`, `_`, `-`, and `.` separators.
 fn is_test_file(path: &str) -> 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);

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<String>,
     pub title: String,
     pub body: String,

src/index/mod.rs

@@ -197,10 +197,16 @@ pub fn index_stats(conn: &Connection) -> anyhow::Result<IndexSummary> {
     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<String, usize> = HashMap::new();