Skip to content

Commit 57d9ce9

Browse files
haasonsaasclaude
andcommitted
fix: 2 TDD bugs — breaking-change false positive, test file detection
1. changelog.rs: `first_line.contains('!')` matched `!` anywhere in the commit description (e.g. "feat: add ! button") causing false breaking-change flags. Fixed by capturing the `!` position in the regex (group 3) and checking `captures.get(3).is_some()` so only the conventional commit `type!:` marker triggers breaking detection. Extracted `parse_conventional_commit_message` for testability. 2. pr_summary.rs: test file detection checked for `.test`/`.spec` extensions, but real test files like `foo.test.js` or `foo_test.rs` have `.js`/`.rs` extensions. Fixed by checking filename patterns (`.test.`, `.spec.`, `_test.`, `_spec`, `test_`) before falling through to extension-based categorization. 23 new tests (1012 total). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 7e18de2 commit 57d9ce9

2 files changed

Lines changed: 360 additions & 15 deletions

File tree

src/core/changelog.rs

Lines changed: 110 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ impl ChangelogGenerator {
9090
pub fn new(repo_path: &str) -> Result<Self> {
9191
let repo = Repository::discover(repo_path)?;
9292
let conventional_regex = Regex::new(
93-
r"^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(([^)]+)\))?(?:!)?:\s*(.+)",
93+
r"^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(([^)]+)\))?(!)?:\s*(.+)",
9494
)?;
9595

9696
Ok(Self {
@@ -152,15 +152,10 @@ impl ChangelogGenerator {
152152

153153
fn parse_commit(&self, commit: &git2::Commit) -> Result<Option<ChangelogEntry>> {
154154
let message = commit.message().unwrap_or("");
155-
let first_line = message.lines().next().unwrap_or("");
156-
157-
// Try to parse as conventional commit
158-
if let Some(captures) = self.conventional_regex.captures(first_line) {
159-
let change_type = ChangeType::from_str(captures.get(1).unwrap().as_str());
160-
let scope = captures.get(2).map(|m| m.as_str().to_string());
161-
let description = captures.get(3).unwrap().as_str().to_string();
162-
let breaking = first_line.contains('!') || message.contains("BREAKING CHANGE");
163155

156+
if let Some((change_type, scope, description, breaking)) =
157+
parse_conventional_commit_message(message, &self.conventional_regex)
158+
{
164159
Ok(Some(ChangelogEntry {
165160
commit_hash: format!("{:.7}", commit.id()),
166161
message: description,
@@ -174,6 +169,7 @@ impl ChangelogGenerator {
174169
}))
175170
} else {
176171
// Non-conventional commit - try to categorize
172+
let first_line = message.lines().next().unwrap_or("");
177173
let change_type = if first_line.to_lowercase().contains("fix") {
178174
ChangeType::Fix
179175
} else if first_line.to_lowercase().contains("add") {
@@ -368,3 +364,108 @@ impl ChangelogGenerator {
368364
output
369365
}
370366
}
367+
368+
/// Parse a conventional commit message into its components.
369+
///
370+
/// Uses the regex with groups: 1=type, 2=scope(opt), 3=`!`(opt), 4=description.
371+
/// Breaking is detected from the `!` marker before `:` or "BREAKING CHANGE" in body,
372+
/// NOT from `!` appearing anywhere in the description text.
373+
fn parse_conventional_commit_message(
374+
message: &str,
375+
conventional_regex: &Regex,
376+
) -> Option<(ChangeType, Option<String>, String, bool)> {
377+
let first_line = message.lines().next().unwrap_or("");
378+
let captures = conventional_regex.captures(first_line)?;
379+
380+
let change_type = ChangeType::from_str(captures.get(1)?.as_str());
381+
let scope = captures.get(2).map(|m| m.as_str().to_string());
382+
let breaking_marker = captures.get(3).is_some();
383+
let description = captures.get(4)?.as_str().to_string();
384+
let breaking = breaking_marker || message.contains("BREAKING CHANGE");
385+
386+
Some((change_type, scope, description, breaking))
387+
}
388+
389+
#[cfg(test)]
390+
mod tests {
391+
use super::*;
392+
393+
fn conventional_regex() -> Regex {
394+
Regex::new(
395+
r"^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(([^)]+)\))?(!)?:\s*(.+)",
396+
)
397+
.unwrap()
398+
}
399+
400+
// ── Bug: `first_line.contains('!')` false positive for breaking changes ──
401+
//
402+
// The old code used `first_line.contains('!')` which matches `!` anywhere
403+
// in the commit message line, including in the description text.
404+
// For example, "feat: add ! button to UI" was wrongly flagged as breaking.
405+
// The conventional commit spec says `!` must appear right before `:` to
406+
// indicate a breaking change (e.g., "feat!: remove old API").
407+
408+
#[test]
409+
fn test_breaking_change_exclamation_in_description_is_not_breaking() {
410+
let re = conventional_regex();
411+
let result = parse_conventional_commit_message("feat: add ! button to UI", &re).unwrap();
412+
assert!(
413+
!result.3,
414+
"Exclamation mark in description text should NOT flag as breaking"
415+
);
416+
}
417+
418+
#[test]
419+
fn test_breaking_change_from_bang_before_colon() {
420+
let re = conventional_regex();
421+
let result =
422+
parse_conventional_commit_message("feat!: remove deprecated API", &re).unwrap();
423+
assert!(result.3, "feat!: should be detected as breaking");
424+
assert_eq!(result.2, "remove deprecated API");
425+
}
426+
427+
#[test]
428+
fn test_breaking_change_from_scoped_bang() {
429+
let re = conventional_regex();
430+
let result =
431+
parse_conventional_commit_message("fix(auth)!: change token format", &re).unwrap();
432+
assert!(result.3, "fix(scope)!: should be detected as breaking");
433+
assert_eq!(result.1.as_deref(), Some("auth"));
434+
}
435+
436+
#[test]
437+
fn test_breaking_change_from_body() {
438+
let re = conventional_regex();
439+
let result = parse_conventional_commit_message(
440+
"feat: new auth flow\n\nBREAKING CHANGE: old tokens are invalid",
441+
&re,
442+
)
443+
.unwrap();
444+
assert!(result.3, "BREAKING CHANGE in body should flag as breaking");
445+
}
446+
447+
#[test]
448+
fn test_not_breaking_regular_commit() {
449+
let re = conventional_regex();
450+
let result = parse_conventional_commit_message("fix: handle edge case", &re).unwrap();
451+
assert!(!result.3, "Regular fix commit should not be breaking");
452+
assert_eq!(result.0, ChangeType::Fix);
453+
}
454+
455+
#[test]
456+
fn test_conventional_commit_with_scope() {
457+
let re = conventional_regex();
458+
let result =
459+
parse_conventional_commit_message("feat(parser): add JSON support", &re).unwrap();
460+
assert_eq!(result.0, ChangeType::Feature);
461+
assert_eq!(result.1.as_deref(), Some("parser"));
462+
assert_eq!(result.2, "add JSON support");
463+
assert!(!result.3);
464+
}
465+
466+
#[test]
467+
fn test_non_conventional_returns_none() {
468+
let re = conventional_regex();
469+
assert!(parse_conventional_commit_message("Update README", &re).is_none());
470+
}
471+
}

0 commit comments

Comments
 (0)