feat(best_practice): add code quality scanning and commit message validation
- Implemented a code quality scanner that checks for common clean-code violations in Rust source files, including missing documentation, usage of `.unwrap()` in production code, and commented-out code. - Introduced a commit message validator that follows the Conventional Commits specification, ensuring proper formatting and providing suggestions for invalid messages. - Created a unified BestPracticeEngine to encapsulate the functionalities of skills, architecture audits, code quality checks, and commit message validation. - Added tests for both the code quality scanner and commit message validator to ensure reliability and correctness.
This commit is contained in:
@@ -0,0 +1,280 @@
|
||||
//! Code-quality analysis for clean-code compliance.
|
||||
//!
|
||||
//! Scans Rust source files for common clean-code violations:
|
||||
//! - Missing doc comments on `pub` items
|
||||
//! - `.unwrap()` / `.expect()` in production (non-test) code
|
||||
//! - `#[allow(...)]` / `#[expect(...)]` compiler bypasses
|
||||
//! - Magic number literals (integer/float constants)
|
||||
//! - Commented-out code blocks
|
||||
//!
|
||||
//! # Flow
|
||||
//!
|
||||
//! `scan_file(path, root)` → read source → classify lines into scopes →
|
||||
//! match each rule → return `Vec<Finding>`.
|
||||
|
||||
use anyhow::Result;
|
||||
use ignore::Walk;
|
||||
use std::path::Path;
|
||||
use tracing::instrument;
|
||||
|
||||
/// A single code-quality finding.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct Finding {
|
||||
pub severity: super::arch_audit::Severity,
|
||||
pub rule: &'static str,
|
||||
pub file: String,
|
||||
pub line: usize,
|
||||
pub message: String,
|
||||
}
|
||||
|
||||
/// Result of a code-quality scan.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct CodeQualityReport {
|
||||
pub findings: Vec<Finding>,
|
||||
pub files_scanned: usize,
|
||||
}
|
||||
|
||||
impl CodeQualityReport {
|
||||
pub fn has_errors(&self) -> bool {
|
||||
self.findings.iter().any(|f| f.severity == super::arch_audit::Severity::Error)
|
||||
}
|
||||
pub fn count_by_rule(&self) -> Vec<(&'static str, usize)> {
|
||||
let mut counts: std::collections::HashMap<&str, usize> = std::collections::HashMap::new();
|
||||
for f in &self.findings {
|
||||
*counts.entry(f.rule).or_default() += 1;
|
||||
}
|
||||
let mut sorted: Vec<_> = counts.into_iter().collect();
|
||||
sorted.sort_by_key(|(_, c)| *c);
|
||||
sorted.reverse();
|
||||
sorted
|
||||
}
|
||||
}
|
||||
|
||||
/// Check if a line is inside a `#[cfg(test)]` block.
|
||||
fn is_in_test_block(content: &[&str], line_idx: usize) -> bool {
|
||||
let mut depth = 0i32;
|
||||
let mut in_test = false;
|
||||
let mut entered_scope = false;
|
||||
for (i, line) in content.iter().enumerate() {
|
||||
if i > line_idx {
|
||||
break;
|
||||
}
|
||||
if line.contains("#[cfg(test)]") {
|
||||
in_test = true;
|
||||
}
|
||||
depth += line.matches('{').count() as i32;
|
||||
if in_test && depth > 0 {
|
||||
entered_scope = true;
|
||||
}
|
||||
depth -= line.matches('}').count() as i32;
|
||||
if in_test && entered_scope && depth == 0 && i < line_idx {
|
||||
in_test = false;
|
||||
entered_scope = false;
|
||||
}
|
||||
}
|
||||
in_test
|
||||
}
|
||||
|
||||
/// Scan a single Rust file for code-quality violations.
|
||||
pub fn scan_quality_file(file_path: &Path, root: &Path) -> Vec<Finding> {
|
||||
let mut findings = Vec::new();
|
||||
let content = match std::fs::read_to_string(file_path) {
|
||||
Ok(c) => c,
|
||||
Err(_) => return findings,
|
||||
};
|
||||
|
||||
let relative = file_path
|
||||
.strip_prefix(root)
|
||||
.unwrap_or(file_path)
|
||||
.to_string_lossy()
|
||||
.to_string();
|
||||
|
||||
let lines: Vec<&str> = content.lines().collect();
|
||||
|
||||
// Track pub items for doc-comment checking.
|
||||
let mut prev_line_doc = false;
|
||||
let mut prev_line_empty = false;
|
||||
|
||||
for (i, line) in lines.iter().enumerate() {
|
||||
let trimmed = line.trim();
|
||||
let line_num = i + 1;
|
||||
let in_test = is_in_test_block(&lines, i);
|
||||
|
||||
// ── Rule: Compiler bypass ──────────────────────────────────────
|
||||
if trimmed.starts_with("#[allow(") || trimmed.starts_with("#[expect(") {
|
||||
// Skip if this is in the workspace lints config (Cargo.toml)
|
||||
if trimmed.contains("clippy::") || trimmed.contains("dead_code") {
|
||||
findings.push(Finding {
|
||||
severity: super::arch_audit::Severity::Error,
|
||||
rule: "compiler-bypass",
|
||||
file: relative.clone(),
|
||||
line: line_num,
|
||||
message: format!("Compiler bypass attribute found: {trimmed}"),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// ── Rule: unwrap / expect in production code ───────────────────
|
||||
if !in_test {
|
||||
if let Some(col) = trimmed.find(".unwrap(") {
|
||||
// Allow unwrap in test code and in `#[]` attributes
|
||||
if !trimmed.starts_with("//") {
|
||||
let snippet = &trimmed[col..(col + 20).min(trimmed.len())];
|
||||
findings.push(Finding {
|
||||
severity: super::arch_audit::Severity::Warning,
|
||||
rule: "unwrap-in-production",
|
||||
file: relative.clone(),
|
||||
line: line_num,
|
||||
message: format!(
|
||||
"Use `?` or proper error handling instead of `.unwrap()`: {snippet}..."
|
||||
),
|
||||
});
|
||||
}
|
||||
}
|
||||
if let Some(col) = trimmed.find(".expect(") {
|
||||
if !trimmed.starts_with("//") {
|
||||
let snippet = &trimmed[col..(col + 20).min(trimmed.len())];
|
||||
findings.push(Finding {
|
||||
severity: super::arch_audit::Severity::Info,
|
||||
rule: "expect-in-production",
|
||||
file: relative.clone(),
|
||||
line: line_num,
|
||||
message: format!(".expect() with message: {snippet}..."),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ── Rule: Missing doc comments on pub items ────────────────────
|
||||
if trimmed.starts_with("pub ") || trimmed.starts_with("pub(") {
|
||||
if !prev_line_doc && !prev_line_empty {
|
||||
// Check it's a struct/enum/fn/trait/type/const/mod
|
||||
let is_item = trimmed.starts_with("pub fn ")
|
||||
|| trimmed.starts_with("pub struct ")
|
||||
|| trimmed.starts_with("pub enum ")
|
||||
|| trimmed.starts_with("pub trait ")
|
||||
|| trimmed.starts_with("pub type ")
|
||||
|| trimmed.starts_with("pub const ")
|
||||
|| trimmed.starts_with("pub mod ")
|
||||
|| trimmed.starts_with("pub(crate) fn ")
|
||||
|| trimmed.starts_with("pub(crate) struct ")
|
||||
|| trimmed.starts_with("pub(crate) enum ")
|
||||
|| trimmed.starts_with("pub(crate) trait ");
|
||||
if is_item {
|
||||
findings.push(Finding {
|
||||
severity: super::arch_audit::Severity::Info,
|
||||
rule: "missing-doc",
|
||||
file: relative.clone(),
|
||||
line: line_num,
|
||||
message: format!("Missing doc comment on pub item: {trimmed}"),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ── Rule: Commented-out code ───────────────────────────────────
|
||||
if trimmed.starts_with("// ") && !in_test {
|
||||
let stripped = trimmed.trim_start_matches("// ");
|
||||
if stripped.starts_with("fn ")
|
||||
|| stripped.starts_with("let ")
|
||||
|| stripped.starts_with("if ")
|
||||
|| stripped.starts_with("for ")
|
||||
|| stripped.starts_with("while ")
|
||||
|| stripped.starts_with("match ")
|
||||
|| stripped.starts_with("pub ")
|
||||
|| stripped.starts_with("impl ")
|
||||
{
|
||||
findings.push(Finding {
|
||||
severity: super::arch_audit::Severity::Info,
|
||||
rule: "commented-code",
|
||||
file: relative.clone(),
|
||||
line: line_num,
|
||||
message: format!("Commented-out code detected: {trimmed}"),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// Track doc-comment state
|
||||
prev_line_doc = trimmed.starts_with("///") || trimmed.starts_with("//!");
|
||||
prev_line_empty = trimmed.is_empty() || trimmed == "//";
|
||||
}
|
||||
|
||||
findings
|
||||
}
|
||||
|
||||
/// Scan an entire workspace for code-quality violations.
|
||||
#[instrument(skip(root))]
|
||||
pub fn scan_quality(root: &Path) -> Result<CodeQualityReport> {
|
||||
let apps_dir = root.join("apps");
|
||||
if !apps_dir.is_dir() {
|
||||
return Ok(CodeQualityReport {
|
||||
findings: vec![],
|
||||
files_scanned: 0,
|
||||
});
|
||||
}
|
||||
|
||||
let mut findings = Vec::new();
|
||||
let mut files_scanned = 0;
|
||||
|
||||
for entry in Walk::new(&apps_dir).flatten() {
|
||||
if entry.file_type().map_or(true, |ft| !ft.is_file()) {
|
||||
continue;
|
||||
}
|
||||
let path = entry.path();
|
||||
if path.extension().map_or(true, |e| e != "rs") {
|
||||
continue;
|
||||
}
|
||||
files_scanned += 1;
|
||||
findings.extend(scan_quality_file(path, root));
|
||||
}
|
||||
|
||||
Ok(CodeQualityReport {
|
||||
findings,
|
||||
files_scanned,
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn detects_unwrap_in_production_code() {
|
||||
let dir = std::env::temp_dir().join(format!("qtest_{}", uuid::Uuid::new_v4()));
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let file = dir.join("test.rs");
|
||||
std::fs::write(&file, "fn x() { let y = foo.unwrap(); }\n").unwrap();
|
||||
|
||||
let findings = scan_quality_file(&file, &dir);
|
||||
let unwrap_findings: Vec<_> = findings.iter().filter(|f| f.rule == "unwrap-in-production").collect();
|
||||
assert!(!unwrap_findings.is_empty(), "should detect unwrap");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skips_unwrap_in_test_block() {
|
||||
let dir = std::env::temp_dir().join(format!("qtest_{}", uuid::Uuid::new_v4()));
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let file = dir.join("test.rs");
|
||||
std::fs::write(
|
||||
&file,
|
||||
"#[cfg(test)]\nmod tests {\n fn x() { let y = foo.unwrap(); }\n}\n",
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let findings = scan_quality_file(&file, &dir);
|
||||
let unwrap_findings: Vec<_> = findings.iter().filter(|f| f.rule == "unwrap-in-production").collect();
|
||||
assert!(unwrap_findings.is_empty(), "should skip unwrap in test blocks");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn detects_allow_attributes() {
|
||||
let dir = std::env::temp_dir().join(format!("qtest_{}", uuid::Uuid::new_v4()));
|
||||
std::fs::create_dir_all(&dir).unwrap();
|
||||
let file = dir.join("test.rs");
|
||||
std::fs::write(&file, "#[allow(clippy::too_many_arguments)]\nfn x() {}\n").unwrap();
|
||||
|
||||
let findings = scan_quality_file(&file, &dir);
|
||||
let bypass_findings: Vec<_> = findings.iter().filter(|f| f.rule == "compiler-bypass").collect();
|
||||
assert!(!bypass_findings.is_empty(), "should detect allow attributes");
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user