Skip to content

Commit abe4dd9

Browse files
committed
Exclude lock files from LLM summarization
- Create a "Lock" category to track files while ignoring content. - Update `prompt_builder` to separate lock files and fix numbering. - Expose `prompt_builder` and update logs to track lock files separately. - Add tests for lock file detection and prompt generation. Token usage: prompt=5689, completion=2950, total=8639
1 parent f2ff1aa commit abe4dd9

6 files changed

Lines changed: 242 additions & 32 deletions

File tree

‎src/lib.rs‎

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,48 @@ pub use git::{
1616
};
1717
pub use llm::LlmClient;
1818

19-
/// How the user categorizes each file in interactive mode.
19+
/// Lock files whose names don't end in `.lock`, so the extension check misses them.
20+
const LOCK_FILE_NAMES: &[&str] = &[
21+
"package-lock.json", // npm
22+
"npm-shrinkwrap.json", // npm
23+
"pnpm-lock.yaml", // pnpm
24+
"bun.lockb", // bun (pre-1.2 binary format)
25+
"packages.lock.json", // NuGet
26+
"gradle.lockfile", // Gradle
27+
];
28+
29+
/// True when the path names a dependency lock file: any `*.lock` file, plus the
30+
/// well-known lock files that use a different extension ([`LOCK_FILE_NAMES`]).
31+
///
32+
/// Lock files are regenerated wholesale by a package manager, so their diffs
33+
/// carry no intent worth asking the LLM about. We skip summarizing them and
34+
/// only tell the final summary that they were touched.
35+
pub fn is_lock_file(path: &str) -> bool {
36+
let path = std::path::Path::new(path);
37+
38+
if path
39+
.extension()
40+
.is_some_and(|ext| ext.eq_ignore_ascii_case("lock"))
41+
{
42+
return true;
43+
}
44+
45+
path.file_name().is_some_and(|name| {
46+
LOCK_FILE_NAMES
47+
.iter()
48+
.any(|known| name.eq_ignore_ascii_case(known))
49+
})
50+
}
51+
52+
/// How each file is categorized. The first four come from the user in
53+
/// interactive mode; `Lock` is assigned automatically by [`is_lock_file`].
2054
#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize)]
2155
pub enum FileCategory {
2256
Main, // 1
2357
Supporting, // 2
2458
Consequence, // 3
2559
Ignored, // 4
60+
Lock, // auto-assigned to *.lock files
2661
}
2762

2863
impl FileCategory {
@@ -33,6 +68,7 @@ impl FileCategory {
3368
FileCategory::Supporting => "supporting",
3469
FileCategory::Consequence => "consequence",
3570
FileCategory::Ignored => "ignored",
71+
FileCategory::Lock => "lock",
3672
}
3773
}
3874
}
@@ -46,6 +82,7 @@ pub struct FileChange {
4682
pub category: FileCategory,
4783
/// Git diff for this file
4884
pub diff: String,
49-
/// LLM-generated summary for this file
85+
/// LLM-generated summary for this file. Always `None` for
86+
/// [`FileCategory::Lock`] and [`FileCategory::Ignored`] files.
5087
pub summary: Option<String>,
5188
}

‎src/llm/mod.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
pub mod ollama;
22
pub mod openai;
3-
mod prompt_builder;
3+
pub mod prompt_builder;
44
mod prompts;
55
mod stream;
66

‎src/llm/prompt_builder.rs‎

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,13 @@ pub fn commit_message_prompt(
5151
}
5252

5353
let per_file = render_per_file_summaries(files);
54-
let file_count = files.len();
54+
let lock_files = render_lock_files_touched(files);
5555
let user = format!(
56-
"Branch: {branch}\n\nFiles Changed: {file_count}\n\nPer-file summaries:\n\n{per_file}",
56+
"Branch: {branch}\n\nFiles Changed: {file_count}\n\nPer-file summaries:\n\n{per_file}{lock_files}",
5757
branch = branch,
58-
file_count = file_count + 1,
59-
per_file = per_file
58+
file_count = files.len(),
59+
per_file = per_file,
60+
lock_files = lock_files
6061
);
6162

6263
PromptPair { system, user }
@@ -147,14 +148,18 @@ pub fn pr_message_prompt(
147148
PromptPair { system, user }
148149
}
149150

151+
/// Renders only the files we actually asked the LLM about, numbered against
152+
/// that subset so the list has no gaps. Ignored and lock files are reported
153+
/// elsewhere (or not at all), not counted here.
150154
fn render_per_file_summaries(files: &[FileChange]) -> String {
151-
let total_files = files.len();
152-
let mut out = String::new();
153-
for (idx, file) in files
155+
let summarized: Vec<&FileChange> = files
154156
.iter()
155-
.enumerate()
156-
.filter(|(_, f)| !matches!(f.category, FileCategory::Ignored))
157-
{
157+
.filter(|f| !matches!(f.category, FileCategory::Ignored | FileCategory::Lock))
158+
.collect();
159+
160+
let total_files = summarized.len();
161+
let mut out = String::new();
162+
for (idx, file) in summarized.iter().enumerate() {
158163
out.push_str(&format!(
159164
"File {file_num} of {total_files}: {path}\nCategory: {category}\nSummary:\n{summary}\n\n",
160165
file_num = idx + 1,
@@ -169,3 +174,27 @@ fn render_per_file_summaries(files: &[FileChange]) -> String {
169174
}
170175
out
171176
}
177+
178+
/// Lock file diffs are never sent to the LLM, so the final summary is told only
179+
/// that they were touched.
180+
fn render_lock_files_touched(files: &[FileChange]) -> String {
181+
let paths: Vec<&str> = files
182+
.iter()
183+
.filter(|f| matches!(f.category, FileCategory::Lock))
184+
.map(|f| f.path.as_str())
185+
.collect();
186+
187+
if paths.is_empty() {
188+
return String::new();
189+
}
190+
191+
let mut out = String::from("Lock files touched/updated (diffs not reviewed):\n");
192+
for path in paths {
193+
out.push_str(&format!("- {path}\n"));
194+
}
195+
out.push_str(
196+
"\nThese were regenerated by a package manager. Mention them only as a dependency \
197+
lock update, and do not speculate about their contents.\n",
198+
);
199+
out
200+
}

‎src/main.rs‎

Lines changed: 53 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,11 @@ fn run_interactive(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> {
306306

307307
let total_files = file_pairs.len();
308308
for (idx, (path, diff)) in file_pairs.into_iter().enumerate() {
309-
let category = categorize_file_interactive(idx, total_files, &path)?;
309+
let category = if commitbot::is_lock_file(&path) {
310+
FileCategory::Lock
311+
} else {
312+
categorize_file_interactive(idx, total_files, &path)?
313+
};
310314
file_changes.push(FileChange {
311315
path,
312316
category,
@@ -329,7 +333,9 @@ fn run_interactive(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> {
329333
.expect("progress style template"),
330334
);
331335
line.set_prefix(fc.path.clone());
332-
if matches!(fc.category, FileCategory::Ignored) {
336+
if matches!(fc.category, FileCategory::Lock) {
337+
line.finish_with_message(dimmed("lock file updated, not summarized"));
338+
} else if matches!(fc.category, FileCategory::Ignored) {
333339
line.finish_with_message(dimmed("ignored"));
334340
} else {
335341
line.enable_steady_tick(Duration::from_millis(120));
@@ -346,20 +352,27 @@ fn run_interactive(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> {
346352

347353
let mut indices_to_summarize = Vec::new();
348354
let mut ignored_count = 0usize;
355+
let mut lock_count = 0usize;
349356

350357
for (idx, fc) in file_changes.iter().enumerate() {
351-
if matches!(fc.category, FileCategory::Ignored) {
352-
pb.inc(1);
353-
ignored_count += 1;
354-
} else {
355-
indices_to_summarize.push(idx);
358+
match fc.category {
359+
FileCategory::Ignored => {
360+
pb.inc(1);
361+
ignored_count += 1;
362+
}
363+
FileCategory::Lock => {
364+
pb.inc(1);
365+
lock_count += 1;
366+
}
367+
_ => indices_to_summarize.push(idx),
356368
}
357369
}
358370

359371
log::info!(
360-
"Summarizing {} files ({} ignored). max_concurrent_requests = {}",
372+
"Summarizing {} files ({} ignored, {} lock). max_concurrent_requests = {}",
361373
indices_to_summarize.len(),
362374
ignored_count,
375+
lock_count,
363376
cfg.max_concurrent_requests,
364377
);
365378

@@ -450,11 +463,18 @@ fn run_auto(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> {
450463

451464
let mut file_changes: Vec<FileChange> = file_pairs
452465
.into_iter()
453-
.map(|(path, diff)| FileChange {
454-
path,
455-
category: FileCategory::Main,
456-
diff,
457-
summary: None,
466+
.map(|(path, diff)| {
467+
let category = if commitbot::is_lock_file(&path) {
468+
FileCategory::Lock
469+
} else {
470+
FileCategory::Main
471+
};
472+
FileChange {
473+
path,
474+
category,
475+
diff,
476+
summary: None,
477+
}
458478
})
459479
.collect();
460480

@@ -481,8 +501,12 @@ fn run_auto(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> {
481501
fc.path.clone()
482502
};
483503
line.set_prefix(prefix);
484-
line.enable_steady_tick(Duration::from_millis(120));
485-
line.set_message("waiting");
504+
if matches!(fc.category, FileCategory::Lock) {
505+
line.finish_with_message(dimmed("lock file updated, not summarized"));
506+
} else {
507+
line.enable_steady_tick(Duration::from_millis(120));
508+
line.set_message("waiting");
509+
}
486510
file_lines.push(line);
487511
}
488512

@@ -492,11 +516,22 @@ fn run_auto(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> {
492516
.unwrap_or_else(|_| ProgressStyle::default_bar()),
493517
);
494518

495-
let indices_to_summarize: Vec<usize> = (0..total).collect();
519+
let mut indices_to_summarize: Vec<usize> = Vec::new();
520+
let mut lock_count = 0usize;
521+
522+
for (idx, fc) in file_changes.iter().enumerate() {
523+
if matches!(fc.category, FileCategory::Lock) {
524+
pb.inc(1);
525+
lock_count += 1;
526+
} else {
527+
indices_to_summarize.push(idx);
528+
}
529+
}
496530

497531
log::info!(
498-
"Auto-summarizing {} files. max_concurrent_requests = {}",
499-
total,
532+
"Auto-summarizing {} files ({} lock). max_concurrent_requests = {}",
533+
indices_to_summarize.len(),
534+
lock_count,
500535
cfg.max_concurrent_requests,
501536
);
502537

‎tests/lib_types.rs‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,37 @@
11
//! Tests for core types and functionality in lib.rs
22
3-
use commitbot::{FileCategory, FileChange};
3+
use commitbot::{is_lock_file, FileCategory, FileChange};
44

55
#[test]
66
fn file_category_str_representation() {
77
assert_eq!(FileCategory::Main.as_str(), "main");
88
assert_eq!(FileCategory::Supporting.as_str(), "supporting");
99
assert_eq!(FileCategory::Consequence.as_str(), "consequence");
1010
assert_eq!(FileCategory::Ignored.as_str(), "ignored");
11+
assert_eq!(FileCategory::Lock.as_str(), "lock");
12+
}
13+
14+
#[test]
15+
fn lock_files_are_detected_by_extension() {
16+
assert!(is_lock_file("Cargo.lock"));
17+
assert!(is_lock_file("yarn.lock"));
18+
assert!(is_lock_file("sub/dir/Gemfile.lock"));
19+
assert!(is_lock_file("composer.LOCK"));
20+
21+
// Lock files that do not use the .lock extension.
22+
assert!(is_lock_file("package-lock.json"));
23+
assert!(is_lock_file("npm-shrinkwrap.json"));
24+
assert!(is_lock_file("pnpm-lock.yaml"));
25+
assert!(is_lock_file("bun.lockb"));
26+
assert!(is_lock_file("packages.lock.json"));
27+
assert!(is_lock_file("gradle.lockfile"));
28+
assert!(is_lock_file("web/frontend/package-lock.json"));
29+
30+
assert!(!is_lock_file("Cargo.toml"));
31+
assert!(!is_lock_file("package.json"));
32+
assert!(!is_lock_file("src/lock.rs"));
33+
assert!(!is_lock_file("lock"));
34+
assert!(!is_lock_file("locked.json"));
1135
}
1236

1337
#[test]

‎tests/prompt_builder.rs‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
//! Tests for the prompts we hand to the LLM.
2+
3+
use commitbot::llm::prompt_builder::commit_message_prompt;
4+
use commitbot::{FileCategory, FileChange};
5+
6+
fn file(path: &str, category: FileCategory, summary: Option<&str>) -> FileChange {
7+
FileChange {
8+
path: path.to_string(),
9+
category,
10+
diff: "diff --git a/x b/x".to_string(),
11+
summary: summary.map(str::to_string),
12+
}
13+
}
14+
15+
#[test]
16+
fn lock_files_are_listed_instead_of_summarized() {
17+
let files = vec![
18+
file("src/main.rs", FileCategory::Main, Some("- Wire up X")),
19+
file("Cargo.lock", FileCategory::Lock, None),
20+
];
21+
22+
let prompt = commit_message_prompt("feature/x", &files, None);
23+
24+
assert!(prompt.user.contains("src/main.rs"));
25+
assert!(prompt.user.contains("- Wire up X"));
26+
assert!(prompt.user.contains("Lock files touched/updated"));
27+
assert!(prompt.user.contains("- Cargo.lock"));
28+
// The lock file never gets a per-file block, so it can't be reported missing.
29+
assert!(!prompt.user.contains("Cargo.lock\nCategory:"));
30+
assert!(!prompt.user.contains("[missing per-file summary]"));
31+
}
32+
33+
#[test]
34+
fn no_lock_section_without_lock_files() {
35+
let files = vec![file("src/main.rs", FileCategory::Main, Some("- Wire up X"))];
36+
37+
let prompt = commit_message_prompt("feature/x", &files, None);
38+
39+
assert!(!prompt.user.contains("Lock files touched/updated"));
40+
}
41+
42+
#[test]
43+
fn ignored_files_stay_out_of_the_lock_section() {
44+
let files = vec![
45+
file("notes.txt", FileCategory::Ignored, None),
46+
file("yarn.lock", FileCategory::Lock, None),
47+
];
48+
49+
let prompt = commit_message_prompt("feature/x", &files, None);
50+
51+
assert!(!prompt.user.contains("notes.txt"));
52+
assert!(prompt.user.contains("- yarn.lock"));
53+
}
54+
55+
#[test]
56+
fn file_count_matches_the_changeset() {
57+
let files = vec![
58+
file("src/main.rs", FileCategory::Main, Some("- Wire up X")),
59+
file("src/lib.rs", FileCategory::Supporting, Some("- Export X")),
60+
file("Cargo.lock", FileCategory::Lock, None),
61+
];
62+
63+
let prompt = commit_message_prompt("feature/x", &files, None);
64+
65+
// Every changed file is counted, including the ones we did not summarize.
66+
assert!(prompt.user.contains("Files Changed: 3"));
67+
}
68+
69+
#[test]
70+
fn per_file_blocks_are_numbered_without_gaps() {
71+
let files = vec![
72+
file("src/main.rs", FileCategory::Main, Some("- Wire up X")),
73+
file("Cargo.lock", FileCategory::Lock, None),
74+
file("notes.txt", FileCategory::Ignored, None),
75+
file("src/lib.rs", FileCategory::Supporting, Some("- Export X")),
76+
];
77+
78+
let prompt = commit_message_prompt("feature/x", &files, None);
79+
80+
// Two files were summarized, so they are "1 of 2" and "2 of 2" — the
81+
// skipped files in between must not leave a hole in the numbering.
82+
assert!(prompt.user.contains("File 1 of 2: src/main.rs"));
83+
assert!(prompt.user.contains("File 2 of 2: src/lib.rs"));
84+
assert!(!prompt.user.contains("of 4"));
85+
}

0 commit comments

Comments
 (0)