Skip to content

Commit 4d23537

Browse files
authored
fix(legacylibrarian): prevent false breaking change positives (#4783)
The separateBodyAndFooters function previously parsed commit lines top-down to find the footer section. This caused body content containing text that matches the footer pattern (such as Renovate release notes documenting breaking changes) to be incorrectly swept into the footers section, leading to false positives for breaking changes. The parser now operates bottom-up from the end of the commit message to identify the true footer section divider line. Additional safety logic prevents the upwards search from erroneously engulfing nested commit headers. Fixes #4779
1 parent b3b994f commit 4d23537

File tree

2 files changed

+85
-21
lines changed

2 files changed

+85
-21
lines changed

internal/legacylibrarian/legacygitrepo/conventional_commits.go

Lines changed: 36 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -320,34 +320,49 @@ func parseHeader(headerLine string) (*parsedHeader, bool) {
320320
}
321321

322322
// separateBodyAndFooters splits the lines after the header into body and footer sections.
323-
// It identifies the footer section by looking for a blank line followed by a
324-
// line that matches the conventional commit footer format.
323+
// It parses lines from the bottom up to identify the true footer section at the end
324+
// of the message, preventing body content (like Renovate release notes) from being
325+
// misidentified as footers. It walks up the last blank line boundary to support
326+
// correctly identifying footers while avoiding consuming nested commit headers.
325327
func separateBodyAndFooters(lines []string) (bodyLines, footerLines []string) {
326-
inFooterSection := false
327-
for i, line := range lines {
328-
if inFooterSection {
329-
footerLines = append(footerLines, line)
330-
continue
328+
lastBlankLine := -1
329+
for i := len(lines) - 1; i >= 0; i-- {
330+
if strings.TrimSpace(lines[i]) == "" {
331+
lastBlankLine = i
332+
break
331333
}
332-
if strings.TrimSpace(line) == "" {
333-
isSeparator := false
334-
// Look ahead at the next non-blank line.
335-
for j := i + 1; j < len(lines); j++ {
336-
if strings.TrimSpace(lines[j]) != "" {
337-
if footerRegex.MatchString(lines[j]) {
338-
isSeparator = true
339-
}
334+
}
335+
336+
if lastBlankLine == -1 {
337+
return lines, nil
338+
}
339+
340+
potentialFooters := lines[lastBlankLine+1:]
341+
if len(potentialFooters) == 0 {
342+
return lines, nil
343+
}
344+
345+
if !footerRegex.MatchString(potentialFooters[0]) {
346+
return lines, nil
347+
}
348+
349+
// Walk up past blank lines to include preceding footers, stopping if we hit a nested commit header.
350+
boundaryIndex := lastBlankLine
351+
for i := lastBlankLine; i >= 0; i-- {
352+
if strings.TrimSpace(lines[i]) == "" {
353+
if i > 0 && footerRegex.MatchString(lines[i-1]) {
354+
if commitRegex.MatchString(lines[i-1]) {
355+
boundaryIndex = i
340356
break
341357
}
358+
continue
342359
}
343-
if isSeparator {
344-
inFooterSection = true
345-
continue // Skip the blank separator line.
346-
}
360+
boundaryIndex = i
361+
break
347362
}
348-
bodyLines = append(bodyLines, line)
349363
}
350-
return bodyLines, footerLines
364+
365+
return lines[:boundaryIndex], lines[boundaryIndex+1:]
351366
}
352367

353368
// parseFooters parses footer lines from a conventional commit message into a map

internal/legacylibrarian/legacygitrepo/conventional_commits_test.go

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -640,6 +640,55 @@ END_NESTED_COMMIT`,
640640
},
641641
},
642642
},
643+
{
644+
name: "renovate_pr_false_positive",
645+
message: "chore(deps): update dependency aip to v0.83.0\n\n" +
646+
"### Release Notes for aip\n\n" +
647+
"BREAKING CHANGE: The HAS operator on timestamp fields now only accepts...\n\n" +
648+
"This PR was generated by Renovate.",
649+
want: []*ConventionalCommit{
650+
{
651+
Type: "chore",
652+
Subject: "update dependency aip to v0.83.0",
653+
Body: "### Release Notes for aip\nBREAKING CHANGE: The HAS operator on timestamp fields now only accepts...\nThis PR was generated by Renovate.",
654+
LibraryID: "example-id",
655+
IsNested: false,
656+
IsBreaking: false,
657+
Footers: map[string]string{},
658+
CommitHash: sha.String(),
659+
When: now,
660+
},
661+
},
662+
},
663+
{
664+
name: "nested_commit_with_breaking_change_footer",
665+
message: "chore: main commit\n\n" +
666+
"BEGIN_NESTED_COMMIT\n" +
667+
"feat: nested feature\n\n" +
668+
"BREAKING CHANGE: nested breaking change\n" +
669+
"END_NESTED_COMMIT",
670+
want: []*ConventionalCommit{
671+
{
672+
Type: "chore",
673+
Subject: "main commit",
674+
LibraryID: "example-id",
675+
IsNested: false,
676+
Footers: map[string]string{},
677+
CommitHash: sha.String(),
678+
When: now,
679+
},
680+
{
681+
Type: "feat",
682+
Subject: "nested feature",
683+
LibraryID: "example-id",
684+
IsNested: true,
685+
IsBreaking: true,
686+
Footers: map[string]string{"BREAKING CHANGE": "nested breaking change"},
687+
CommitHash: sha.String(),
688+
When: now,
689+
},
690+
},
691+
},
643692
} {
644693
t.Run(test.name, func(t *testing.T) {
645694
commit := &Commit{

0 commit comments

Comments
 (0)