diff --git a/changelog_binding_test.go b/changelog_binding_test.go new file mode 100644 index 0000000..615803a --- /dev/null +++ b/changelog_binding_test.go @@ -0,0 +1,73 @@ +package specsync + +import "testing" + +// Pins how BuildChangelog binds a commit to a change, because the two cases +// below are indistinguishable in the gate's error message and are not the same +// situation: +// +// - "(#42)" in a squash-merge header, where 42 IS the change's issue id. +// extractRefs files a header "(#N)" under PRRefs, and BuildChangelog +// consults append(IssueRefs, PRRefs...) against issueToChange, so this DOES +// bind. The form the gate recommends is the form that works. +// - "(#131)" where 131 is a PR number and not any change's issue. Nothing +// binds, and the commit is correctly reported as loose. +// +// A reader of the error message cannot tell these apart: both print a "(#N)" +// and both are rejected or accepted on grounds the message never states. That +// ambiguity is what makes 57c7e49 look self-contradictory rather than merely +// unlinked. +func TestBindingDistinguishesHeaderRefFromPRRef(t *testing.T) { + t.Run("header ref binds when the number is the change's issue", func(t *testing.T) { + in := TraceInput{ + Commits: []Commit{{ + Hash: "abc1234", Type: "feat", ConventionalOK: true, + Description: "something", PRRefs: []string{"#42"}, + }}, + Changes: []ChangeRefs{ + {Change: Change{Slug: "my-change"}, IssueIDs: []string{"42"}}, + }, + } + cl := BuildChangelog(in, nil) + if !hasBoundEntry(cl, "my-change") { + t.Fatalf("header (#42) should bind to the change whose issue is 42; entries=%+v", cl.Entries) + } + }) + + t.Run("header ref does not bind when the number is only a PR", func(t *testing.T) { + in := TraceInput{ + Commits: []Commit{{ + Hash: "57c7e49", Type: "feat", ConventionalOK: true, + Description: "something", PRRefs: []string{"#131"}, + }}, + Changes: []ChangeRefs{ + {Change: Change{Slug: "my-change"}, IssueIDs: []string{"42"}}, + }, + } + cl := BuildChangelog(in, nil) + if hasBoundEntry(cl, "my-change") { + t.Fatal("(#131) is not this change's issue and must not bind") + } + if !hasLooseEntry(cl, "57c7e49") { + t.Fatalf("expected a loose entry for the unbound commit; entries=%+v", cl.Entries) + } + }) +} + +func hasBoundEntry(cl Changelog, slug string) bool { + for _, e := range cl.Entries { + if e.Slug == slug && e.Hash == "" { + return true + } + } + return false +} + +func hasLooseEntry(cl Changelog, hash string) bool { + for _, e := range cl.Entries { + if e.Hash == hash && e.Slug == "" { + return true + } + } + return false +} diff --git a/cmd/specsync/changelog.go b/cmd/specsync/changelog.go index 6c92cfa..576c0a9 100644 --- a/cmd/specsync/changelog.go +++ b/cmd/specsync/changelog.go @@ -136,7 +136,7 @@ func unlinkedCommitsError(cl specsync.Changelog, failOnUnlinkedCommits bool) err if len(offenders) == 0 { return nil } - return fmt.Errorf("changelog: %d commit(s) not linked to a change's issue: %s — reference the issue (e.g. \"(#42)\", \"Closes #42\") so this renders as an authored release note instead of a raw commit line", + return fmt.Errorf("changelog: %d commit(s) not linked to a change's issue: %s — each needs the issue number of the change it ships, in one of two forms. Either reference it in the commit message (e.g. \"(#42)\", \"Closes #42\") so this renders as an authored release note instead of a raw commit line, or — when the commit message is not yours to edit, as with a squash-merge — add a \"## Release note\" section to the change's proposal.md, which is the only remedy available in that case. Note that a \"(#N)\" in a squash-merge header is often the PR number rather than the change's issue; that is why 131-style refs above can look present and still be unlinked. If the number is the change's issue, \"(#42)\" does bind and nothing else is needed", len(offenders), strings.Join(offenders, ", ")) } diff --git a/cmd/specsync/changelog_test.go b/cmd/specsync/changelog_test.go index 534856f..8cc43ed 100644 --- a/cmd/specsync/changelog_test.go +++ b/cmd/specsync/changelog_test.go @@ -70,3 +70,49 @@ func TestUnlinkedCommitsErrorIgnoresSilentlyOmittedCommits(t *testing.T) { t.Fatalf("expected nil — nothing for the gate to flag, got %v", err) } } + +// TestUnlinkedCommitsErrorNamesEveryAvailableRemedy pins the gate's message +// against the failure it caused: the message used to recommend only forms that +// require editing the commit message, which is impossible for a squash-merge. +// A reader hitting that case had no stated remedy and, because the rejected +// commit still printed a "(#N)", no way to tell the gate was right. +// +// Each assertion below fails if its sentence is removed, which is the point: +// the message is the only documentation an author has at the moment they are +// blocked by this gate. +func TestUnlinkedCommitsErrorNamesEveryAvailableRemedy(t *testing.T) { + cl := specsync.Changelog{Entries: []specsync.ChangelogEntry{ + {Text: "unify agent skill integration", Hash: "57c7e49"}, + }} + err := unlinkedCommitsError(cl, true) + if err == nil { + t.Fatal("expected an error for an unlinked commit") + } + msg := err.Error() + + t.Run("names the proposal remedy for commits you cannot edit", func(t *testing.T) { + if !strings.Contains(msg, "## Release note") { + t.Fatalf("message must name the only remedy available to a squash-merge author; got %q", msg) + } + if !strings.Contains(msg, "squash-merge") { + t.Fatalf("message must say when the proposal remedy is the applicable one; got %q", msg) + } + }) + + t.Run("explains why a printed ref can still be unlinked", func(t *testing.T) { + if !strings.Contains(msg, "PR number") { + t.Fatalf("message must distinguish a PR ref from an issue ref; got %q", msg) + } + }) + + // "(#42)" is retained deliberately. It looks like the form to distrust, + // because extractRefs files a header "(#N)" under PRRefs — but BuildChangelog + // consults PRRefs too, so "(#42)" binds whenever 42 is the change's issue. + // See TestBindingDistinguishesHeaderRefFromPRRef. Dropping it from the + // examples would remove correct advice, so pin its presence. + t.Run("keeps the header form that does bind", func(t *testing.T) { + if !strings.Contains(msg, "(#42)") { + t.Fatalf(`message must keep "(#42)": it binds when 42 is the change's issue; got %q`, msg) + } + }) +}