fix: name every available remedy in the unlinked-commit gate message

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
+}

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, ", "))
 }
 

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)
+		}
+	})
+}