diff --git a/src/lib.rs b/src/lib.rs index bcbf502..a525a10 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,13 +16,48 @@ pub use git::{ }; pub use llm::LlmClient; -/// How the user categorizes each file in interactive mode. +/// Lock files whose names don't end in `.lock`, so the extension check misses them. +const LOCK_FILE_NAMES: &[&str] = &[ + "package-lock.json", // npm + "npm-shrinkwrap.json", // npm + "pnpm-lock.yaml", // pnpm + "bun.lockb", // bun (pre-1.2 binary format) + "packages.lock.json", // NuGet + "gradle.lockfile", // Gradle +]; + +/// True when the path names a dependency lock file: any `*.lock` file, plus the +/// well-known lock files that use a different extension ([`LOCK_FILE_NAMES`]). +/// +/// Lock files are regenerated wholesale by a package manager, so their diffs +/// carry no intent worth asking the LLM about. We skip summarizing them and +/// only tell the final summary that they were touched. +pub fn is_lock_file(path: &str) -> bool { + let path = std::path::Path::new(path); + + if path + .extension() + .is_some_and(|ext| ext.eq_ignore_ascii_case("lock")) + { + return true; + } + + path.file_name().is_some_and(|name| { + LOCK_FILE_NAMES + .iter() + .any(|known| name.eq_ignore_ascii_case(known)) + }) +} + +/// How each file is categorized. The first four come from the user in +/// interactive mode; `Lock` is assigned automatically by [`is_lock_file`]. #[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize)] pub enum FileCategory { Main, // 1 Supporting, // 2 Consequence, // 3 Ignored, // 4 + Lock, // auto-assigned to *.lock files } impl FileCategory { @@ -33,6 +68,7 @@ impl FileCategory { FileCategory::Supporting => "supporting", FileCategory::Consequence => "consequence", FileCategory::Ignored => "ignored", + FileCategory::Lock => "lock", } } } @@ -46,6 +82,7 @@ pub struct FileChange { pub category: FileCategory, /// Git diff for this file pub diff: String, - /// LLM-generated summary for this file + /// LLM-generated summary for this file. Always `None` for + /// [`FileCategory::Lock`] and [`FileCategory::Ignored`] files. pub summary: Option, } diff --git a/src/llm/mod.rs b/src/llm/mod.rs index e352a1e..68d6b2a 100644 --- a/src/llm/mod.rs +++ b/src/llm/mod.rs @@ -1,6 +1,6 @@ pub mod ollama; pub mod openai; -mod prompt_builder; +pub mod prompt_builder; mod prompts; mod stream; diff --git a/src/llm/prompt_builder.rs b/src/llm/prompt_builder.rs index e203886..dcfc57c 100644 --- a/src/llm/prompt_builder.rs +++ b/src/llm/prompt_builder.rs @@ -51,12 +51,13 @@ pub fn commit_message_prompt( } let per_file = render_per_file_summaries(files); - let file_count = files.len(); + let lock_files = render_lock_files_touched(files); let user = format!( - "Branch: {branch}\n\nFiles Changed: {file_count}\n\nPer-file summaries:\n\n{per_file}", + "Branch: {branch}\n\nFiles Changed: {file_count}\n\nPer-file summaries:\n\n{per_file}{lock_files}", branch = branch, - file_count = file_count + 1, - per_file = per_file + file_count = files.len(), + per_file = per_file, + lock_files = lock_files ); PromptPair { system, user } @@ -147,14 +148,18 @@ pub fn pr_message_prompt( PromptPair { system, user } } +/// Renders only the files we actually asked the LLM about, numbered against +/// that subset so the list has no gaps. Ignored and lock files are reported +/// elsewhere (or not at all), not counted here. fn render_per_file_summaries(files: &[FileChange]) -> String { - let total_files = files.len(); - let mut out = String::new(); - for (idx, file) in files + let summarized: Vec<&FileChange> = files .iter() - .enumerate() - .filter(|(_, f)| !matches!(f.category, FileCategory::Ignored)) - { + .filter(|f| !matches!(f.category, FileCategory::Ignored | FileCategory::Lock)) + .collect(); + + let total_files = summarized.len(); + let mut out = String::new(); + for (idx, file) in summarized.iter().enumerate() { out.push_str(&format!( "File {file_num} of {total_files}: {path}\nCategory: {category}\nSummary:\n{summary}\n\n", file_num = idx + 1, @@ -169,3 +174,27 @@ fn render_per_file_summaries(files: &[FileChange]) -> String { } out } + +/// Lock file diffs are never sent to the LLM, so the final summary is told only +/// that they were touched. +fn render_lock_files_touched(files: &[FileChange]) -> String { + let paths: Vec<&str> = files + .iter() + .filter(|f| matches!(f.category, FileCategory::Lock)) + .map(|f| f.path.as_str()) + .collect(); + + if paths.is_empty() { + return String::new(); + } + + let mut out = String::from("Lock files touched/updated (diffs not reviewed):\n"); + for path in paths { + out.push_str(&format!("- {path}\n")); + } + out.push_str( + "\nThese were regenerated by a package manager. Mention them only as a dependency \ + lock update, and do not speculate about their contents.\n", + ); + out +} diff --git a/src/main.rs b/src/main.rs index 714085e..0f2c937 100644 --- a/src/main.rs +++ b/src/main.rs @@ -306,7 +306,11 @@ fn run_interactive(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { let total_files = file_pairs.len(); for (idx, (path, diff)) in file_pairs.into_iter().enumerate() { - let category = categorize_file_interactive(idx, total_files, &path)?; + let category = if commitbot::is_lock_file(&path) { + FileCategory::Lock + } else { + categorize_file_interactive(idx, total_files, &path)? + }; file_changes.push(FileChange { path, category, @@ -329,7 +333,9 @@ fn run_interactive(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { .expect("progress style template"), ); line.set_prefix(fc.path.clone()); - if matches!(fc.category, FileCategory::Ignored) { + if matches!(fc.category, FileCategory::Lock) { + line.finish_with_message(dimmed("lock file updated, not summarized")); + } else if matches!(fc.category, FileCategory::Ignored) { line.finish_with_message(dimmed("ignored")); } else { line.enable_steady_tick(Duration::from_millis(120)); @@ -346,20 +352,27 @@ fn run_interactive(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { let mut indices_to_summarize = Vec::new(); let mut ignored_count = 0usize; + let mut lock_count = 0usize; for (idx, fc) in file_changes.iter().enumerate() { - if matches!(fc.category, FileCategory::Ignored) { - pb.inc(1); - ignored_count += 1; - } else { - indices_to_summarize.push(idx); + match fc.category { + FileCategory::Ignored => { + pb.inc(1); + ignored_count += 1; + } + FileCategory::Lock => { + pb.inc(1); + lock_count += 1; + } + _ => indices_to_summarize.push(idx), } } log::info!( - "Summarizing {} files ({} ignored). max_concurrent_requests = {}", + "Summarizing {} files ({} ignored, {} lock). max_concurrent_requests = {}", indices_to_summarize.len(), ignored_count, + lock_count, cfg.max_concurrent_requests, ); @@ -450,11 +463,18 @@ fn run_auto(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { let mut file_changes: Vec = file_pairs .into_iter() - .map(|(path, diff)| FileChange { - path, - category: FileCategory::Main, - diff, - summary: None, + .map(|(path, diff)| { + let category = if commitbot::is_lock_file(&path) { + FileCategory::Lock + } else { + FileCategory::Main + }; + FileChange { + path, + category, + diff, + summary: None, + } }) .collect(); @@ -481,8 +501,12 @@ fn run_auto(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { fc.path.clone() }; line.set_prefix(prefix); - line.enable_steady_tick(Duration::from_millis(120)); - line.set_message("waiting"); + if matches!(fc.category, FileCategory::Lock) { + line.finish_with_message(dimmed("lock file updated, not summarized")); + } else { + line.enable_steady_tick(Duration::from_millis(120)); + line.set_message("waiting"); + } file_lines.push(line); } @@ -492,11 +516,22 @@ fn run_auto(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { .unwrap_or_else(|_| ProgressStyle::default_bar()), ); - let indices_to_summarize: Vec = (0..total).collect(); + let mut indices_to_summarize: Vec = Vec::new(); + let mut lock_count = 0usize; + + for (idx, fc) in file_changes.iter().enumerate() { + if matches!(fc.category, FileCategory::Lock) { + pb.inc(1); + lock_count += 1; + } else { + indices_to_summarize.push(idx); + } + } log::info!( - "Auto-summarizing {} files. max_concurrent_requests = {}", - total, + "Auto-summarizing {} files ({} lock). max_concurrent_requests = {}", + indices_to_summarize.len(), + lock_count, cfg.max_concurrent_requests, ); diff --git a/tests/lib_types.rs b/tests/lib_types.rs index ec4b102..d13de98 100644 --- a/tests/lib_types.rs +++ b/tests/lib_types.rs @@ -1,6 +1,6 @@ //! Tests for core types and functionality in lib.rs -use commitbot::{FileCategory, FileChange}; +use commitbot::{is_lock_file, FileCategory, FileChange}; #[test] fn file_category_str_representation() { @@ -8,6 +8,30 @@ fn file_category_str_representation() { assert_eq!(FileCategory::Supporting.as_str(), "supporting"); assert_eq!(FileCategory::Consequence.as_str(), "consequence"); assert_eq!(FileCategory::Ignored.as_str(), "ignored"); + assert_eq!(FileCategory::Lock.as_str(), "lock"); +} + +#[test] +fn lock_files_are_detected_by_extension() { + assert!(is_lock_file("Cargo.lock")); + assert!(is_lock_file("yarn.lock")); + assert!(is_lock_file("sub/dir/Gemfile.lock")); + assert!(is_lock_file("composer.LOCK")); + + // Lock files that do not use the .lock extension. + assert!(is_lock_file("package-lock.json")); + assert!(is_lock_file("npm-shrinkwrap.json")); + assert!(is_lock_file("pnpm-lock.yaml")); + assert!(is_lock_file("bun.lockb")); + assert!(is_lock_file("packages.lock.json")); + assert!(is_lock_file("gradle.lockfile")); + assert!(is_lock_file("web/frontend/package-lock.json")); + + assert!(!is_lock_file("Cargo.toml")); + assert!(!is_lock_file("package.json")); + assert!(!is_lock_file("src/lock.rs")); + assert!(!is_lock_file("lock")); + assert!(!is_lock_file("locked.json")); } #[test] diff --git a/tests/prompt_builder.rs b/tests/prompt_builder.rs new file mode 100644 index 0000000..18eafe8 --- /dev/null +++ b/tests/prompt_builder.rs @@ -0,0 +1,85 @@ +//! Tests for the prompts we hand to the LLM. + +use commitbot::llm::prompt_builder::commit_message_prompt; +use commitbot::{FileCategory, FileChange}; + +fn file(path: &str, category: FileCategory, summary: Option<&str>) -> FileChange { + FileChange { + path: path.to_string(), + category, + diff: "diff --git a/x b/x".to_string(), + summary: summary.map(str::to_string), + } +} + +#[test] +fn lock_files_are_listed_instead_of_summarized() { + let files = vec![ + file("src/main.rs", FileCategory::Main, Some("- Wire up X")), + file("Cargo.lock", FileCategory::Lock, None), + ]; + + let prompt = commit_message_prompt("feature/x", &files, None); + + assert!(prompt.user.contains("src/main.rs")); + assert!(prompt.user.contains("- Wire up X")); + assert!(prompt.user.contains("Lock files touched/updated")); + assert!(prompt.user.contains("- Cargo.lock")); + // The lock file never gets a per-file block, so it can't be reported missing. + assert!(!prompt.user.contains("Cargo.lock\nCategory:")); + assert!(!prompt.user.contains("[missing per-file summary]")); +} + +#[test] +fn no_lock_section_without_lock_files() { + let files = vec![file("src/main.rs", FileCategory::Main, Some("- Wire up X"))]; + + let prompt = commit_message_prompt("feature/x", &files, None); + + assert!(!prompt.user.contains("Lock files touched/updated")); +} + +#[test] +fn ignored_files_stay_out_of_the_lock_section() { + let files = vec![ + file("notes.txt", FileCategory::Ignored, None), + file("yarn.lock", FileCategory::Lock, None), + ]; + + let prompt = commit_message_prompt("feature/x", &files, None); + + assert!(!prompt.user.contains("notes.txt")); + assert!(prompt.user.contains("- yarn.lock")); +} + +#[test] +fn file_count_matches_the_changeset() { + let files = vec![ + file("src/main.rs", FileCategory::Main, Some("- Wire up X")), + file("src/lib.rs", FileCategory::Supporting, Some("- Export X")), + file("Cargo.lock", FileCategory::Lock, None), + ]; + + let prompt = commit_message_prompt("feature/x", &files, None); + + // Every changed file is counted, including the ones we did not summarize. + assert!(prompt.user.contains("Files Changed: 3")); +} + +#[test] +fn per_file_blocks_are_numbered_without_gaps() { + let files = vec![ + file("src/main.rs", FileCategory::Main, Some("- Wire up X")), + file("Cargo.lock", FileCategory::Lock, None), + file("notes.txt", FileCategory::Ignored, None), + file("src/lib.rs", FileCategory::Supporting, Some("- Export X")), + ]; + + let prompt = commit_message_prompt("feature/x", &files, None); + + // Two files were summarized, so they are "1 of 2" and "2 of 2" — the + // skipped files in between must not leave a hole in the numbering. + assert!(prompt.user.contains("File 1 of 2: src/main.rs")); + assert!(prompt.user.contains("File 2 of 2: src/lib.rs")); + assert!(!prompt.user.contains("of 4")); +}