From 1a2c172f8e65d4af776dd5b3dd4756f5be395fde Mon Sep 17 00:00:00 2001 From: AndreasS Date: Mon, 28 Sep 2026 01:59:37 +0200 Subject: [PATCH] fix: name every available remedy in the unlinked-commit gate message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate is correct — it flags exactly the commits that cannot bind to a change's issue. The message was not: it recommended only forms that require editing the commit message, which is impossible for a squash-merge, and it omitted the ## Release note remedy that AGENTS.md already documents and that is the only option when the message is generated by the merge. It also never surfaced the ref's kind, so a commit ending in a PR number prints a ref next to its own rejection. That reads as self-contradictory and sends readers to suspect the gate rather than the commit. Keeps "(#42)" in the examples deliberately. It is filed under PRRefs by extractRefs, which makes it look untrustworthy, but BuildChangelog consults PRRefs too, so it binds whenever 42 is the change's issue. Pinned by TestBindingDistinguishesHeaderRefFromPRRef so a future edit cannot drop correct advice on the mistaken reasoning that header refs never bind. Message-only change; no detection logic touched. Both new tests fail when the message is reverted. (#119) --- changelog_binding_test.go | 73 ++++++++++++++++++++++++++++++++++ cmd/specsync/changelog.go | 2 +- cmd/specsync/changelog_test.go | 46 +++++++++++++++++++++ 3 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 changelog_binding_test.go 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) + } + }) +}