docs: strengthen breaking-change marking guidance

[ralphbean]

Summary

• Expand COMMITS.md "Breaking changes" from a one-liner into a full section: rationale, marking instructions, a "how to tell" checklist, and a complete example commit message
• Add missing-! verification to the "Reviewing commit messages and PR titles" section as an important-severity finding
• Add a breaking-change callout to CONTRIBUTING.md's "Opening a PR" checklist
• Strengthen the COMMITS.md reference in AGENTS.md to explicitly cover PR titles and the ! requirement

Builds on #2627 (use: github in GoReleaser) — PR titles now drive release notes, so the ! marker on PR titles is especially critical.

Closes #2654

Test plan

• make lint passes
• Review rendered markdown in the PR files tab

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

docs: strengthen breaking-change marking guidance
📝 Documentation 🕐 10-20 Minutes

Description

• Expand breaking-change guidance to require ! in both commits and PR titles.
• Add a reviewer checklist item to flag missing ! as important severity.
• Update contributor/agent docs to reinforce GoReleaser PR-title release note behavior.

Diagram

graph TD
  Contributor(["Contributor"]) --> Contributing["CONTRIBUTING.md"] --> PRTitle["PR title & commits"] --> Reviewer(["Reviewer"]) --> GoReleaser["GoReleaser release notes"]
  PRTitle --> Commits["COMMITS.md"]
  Agents["AGENTS.md"] --> Commits

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Add automated PR-title/commit linting in CI

• ➕ Prevents missing ! (and bad prefixes) from merging
• ➕ Reduces reviewer burden and inconsistent enforcement
• ➕ Creates a single source of truth for rules
• ➖ Hard to automatically detect whether a change is truly breaking from diffs alone
• ➖ May produce false positives/negatives without a structured input (labels/checkbox)

2. PR template checkbox for breaking changes + required `!` when checked

• ➕ Low-effort workflow change; makes intent explicit
• ➕ Enables simple automation: if checked, enforce ! in title
• ➖ Relies on authors to correctly self-identify breaking changes
• ➖ Doesn’t enforce the BREAKING CHANGE: trailer in commits unless also checked

3. Adopt conventional-commits tooling (e.g., commitlint/semantic-pull-requests)

• ➕ Standardized ecosystem and existing GitHub Actions
• ➕ Can enforce prefix/scope formatting and presence of ! when used
• ➖ Adds dependency and maintenance overhead
• ➖ Still needs a policy decision for how to determine “breaking” vs “non-breaking”

Recommendation: This docs-first approach is appropriate as an immediate fix given the incident history and the GoReleaser PR-title dependency. Consider a follow-up to add lightweight automation (PR template checkbox + action) to enforce ! when authors declare a breaking change; fully automated “break detection” from diffs is likely too error-prone.

Files changed (3) +38 / -4

Documentation (3) +38 / -4

AGENTS.mdRequire COMMITS.md rules for PR titles and breaking-change '!' +1/-1

Require COMMITS.md rules for PR titles and breaking-change '!'

• Extends the existing commit-message guidance to explicitly include PR titles. Calls out the '!' suffix requirement for breaking changes in both commits and PR titles, and frames missing '!' as an important-severity review finding due to GoReleaser release-note generation.

AGENTS.md

COMMITS.mdExpand breaking-change section and strengthen reviewer checklist +36/-3

Expand breaking-change section and strengthen reviewer checklist

• Replaces the prior one-line breaking-change note with a full section: rationale tied to GoReleaser PR-title release notes, step-by-step marking requirements, a checklist for identifying breaking changes, and a complete example commit message. Updates the review section to explicitly cover PR titles and to treat missing '!' on breaking changes as an important-severity issue.

COMMITS.md

CONTRIBUTING.mdAdd PR checklist callout for breaking-change '!' in titles +1/-0

Add PR checklist callout for breaking-change '!' in titles

• Adds an explicit checklist item warning that breaking changes must be marked with '!' in the PR title because release notes are built from PR titles. Links to COMMITS.md for identification criteria and required commit-body trailer content.

CONTRIBUTING.md

[github-actions]

Site preview

Preview: https://1530d135-site.fullsend-ai.workers.dev

Commit: c38d0ac4c08235dd57311783f04b8b2c38117f41

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:14 PM UTC · Completed 2:24 PM UTC
Commit: 4373d68 · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 ADR-0045 Phase 4 PR #2446 shipped breaking change without ! marker

✅ Compliance rules (platform): 51 rules

✅ Skills: writing-user-docs, writing-adrs

1. Release-note source mismatch 🐞 Bug ⚙ Maintainability

Description

COMMITS.md now says GoReleaser builds release notes from merged PR titles, but the file header still
claims every commit on main feeds the release notes. This contradiction can confuse contributors
about whether PR titles or commit subjects are the authoritative text to format correctly.

Code

COMMITS.md[R55-56]

+Breaking changes **must** be marked in both commit messages and PR titles. GoReleaser builds release notes from merged PR titles (`use: github` in `.goreleaser.yml`), so an unmarked PR title means the breaking change is invisible to users reading the release notes. This has caused real incidents — users upgraded with no warning that their agents would stop working.
+

Relevance

⭐⭐⭐ High

Team often accepts resolving doc contradictions/mismatches; e.g., accepted contradiction fix in
#286; COMMITS.md is single source (#2089).

PR-#286
PR-#2089
PR-#2627

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

COMMITS.md’s intro asserts commit-driven release notes, while the newly added breaking-changes
section asserts PR-title-driven release notes; .goreleaser.yml confirms PR-title sourcing via
changelog.use: github.

COMMITS.md[1-4]
COMMITS.md[55-56]
.goreleaser.yml[34-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`COMMITS.md` now states that GoReleaser builds release notes from merged PR titles, but the header still says every commit on `main` feeds the release notes. These statements conflict and can mislead contributors about what artifact (PR title vs commit subject) must follow the Conventional Commits format.

### Issue Context
- GoReleaser is configured with `changelog.use: github`, which matches the new PR-title guidance.
- The file intro should be updated to reflect the PR-title-based source of changelog entries (or explicitly describe the relationship between PR titles and resulting commits, if relevant).

### Fix Focus Areas
- COMMITS.md[1-4]
- COMMITS.md[55-56]
- .goreleaser.yml[34-36]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Medium

• [internal-consistency] COMMITS.md:3 — Line 3 states "Every commit on main feeds the auto-generated release notes (via GoReleaser)" but the new text added by this PR establishes that GoReleaser builds release notes from merged PR titles, not individual commit subjects (confirmed by changelog.use: github in .goreleaser.yml). This creates a self-contradiction within the same file.
  Remediation: Update line 3 to say something like "Every merged PR title feeds the auto-generated release notes (via GoReleaser), so getting the prefix right matters." to align with the rest of the file and the actual GoReleaser configuration.

• [terminology-consistency] COMMITS.md:107, AGENTS.md:14 — The phrase "important-severity finding" uses non-standard severity terminology. The codebase consistently uses specific severity levels (critical, high, medium, low, info) when referring to finding severity — e.g., "medium-severity finding" in AGENTS.md and "high-severity" in review docs. The term "important" does not appear in the established severity vocabulary anywhere in the repository.
  Remediation: Replace "important-severity finding" with a specific severity level (e.g., "high-severity finding") in both COMMITS.md and AGENTS.md.

• [protected-path] AGENTS.md, skills/cutting-releases/SKILL.md — This PR modifies protected governance files (AGENTS.md and files under skills/). The change is justified by linked issue ADR-0045 Phase 4 PR #2446 shipped breaking change without ! marker #2654 and the PR description explains the rationale. Human approval is required for protected-path changes regardless of context.

---

Labels: PR modifies contributor-facing documentation (COMMITS.md, CONTRIBUTING.md, AGENTS.md) and a skill file under skills/.

Previous run

Review

Findings

Medium

• [technical accuracy] COMMITS.md:61 — The documentation states "The ! suffix is what GoReleaser and tooling can match on" but .goreleaser.yml does not have a dedicated "Breaking Changes" changelog group. The existing regexes (e.g., '^.*?feat(\([^)]+\))??!?:.+$') treat ! as optional and sort breaking changes into the same groups as non-breaking ones (Features, Bug Fixes, etc.). GoReleaser is not currently configured to surface breaking changes distinctly based on the ! marker. The claim is aspirational rather than accurate for the current configuration.
  Remediation: Either (a) add a "Breaking Changes" group to .goreleaser.yml with a regex matching the ! suffix, or (b) soften the wording to say the ! suffix is for human reviewers and future tooling rather than implying GoReleaser currently acts on it.

• [stale-doc] skills/cutting-releases/SKILL.md:134 — States "The changelog is auto-generated from conventional commit prefixes" but this PR clarifies that GoReleaser uses PR titles (with use: github in .goreleaser.yml) to build release notes. This pre-existing imprecision is now inconsistent with the updated guidance in COMMITS.md and AGENTS.md.
  Remediation: Update to state that the changelog is auto-generated from PR titles (which must follow conventional commit format).

• [protected-path] AGENTS.md — This PR modifies a protected governance file (AGENTS.md). The change is justified by linked issue ADR-0045 Phase 4 PR #2446 shipped breaking change without ! marker #2654 and the PR description explains the rationale. Human approval is required for protected-path changes regardless of context.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] technical accuracy

The documentation states 'The ! suffix is what GoReleaser and tooling can match on' but .goreleaser.yml does not have a dedicated 'Breaking Changes' changelog group. The existing regexes treat ! as optional and sort breaking changes into the same groups as non-breaking ones. GoReleaser is not currently configured to surface breaking changes distinctly based on the ! marker.

Suggested fix: Either (a) add a 'Breaking Changes' group to .goreleaser.yml with a regex matching the ! suffix, or (b) soften the wording to say the ! suffix is for human reviewers and future tooling rather than implying GoReleaser currently acts on it.

[ralphbean]

/fs-fix fix the stale-docs and technical accuracy findings

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 2:53 PM UTC · Completed 2:58 PM UTC
Commit: 2d8adb7 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Fixed 2 of 3 review findings (technical accuracy and stale-doc). The protected-path finding is informational and requires human approval — no code change needed.

Fixed (2):

1. technical accuracy: COMMITS.md claims GoReleaser matches on ! suffix (COMMITS.md): Softened wording from 'The ! suffix is what GoReleaser and tooling can match on' to 'The ! suffix signals the breaking change to human reviewers and enables future automated tooling'. The current .goreleaser.yml treats ! as optional in regexes and has no dedicated Breaking Changes group, so the original claim was aspirational.
2. stale-doc: cutting-releases skill says changelog from conventional commit prefixes (skills/cutting-releases/SKILL.md): Updated changelog note from 'auto-generated from conventional commit prefixes' to 'auto-generated from PR titles (which must follow conventional commit format)' with an explanation that GoReleaser uses changelog.use: github, so merged PR titles are the source.

Disagreed (1):

1. protected-path: AGENTS.md modification requires human approval: This is an informational finding, not a code change request. The AGENTS.md change was already made in a prior commit and is justified by linked issue ADR-0045 Phase 4 PR #2446 shipped breaking change without ! marker #2654. Human approval is a process requirement handled outside this fix agent.

Tests: passed

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 3:02 PM UTC · Completed 3:32 PM UTC
Commit: c38d0ac · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 5:29 PM UTC · Completed 5:38 PM UTC
Commit: c38d0ac · View workflow run →

[fullsend-ai-retro]

Retro: PR #2657 — docs: strengthen breaking-change marking guidance

Workflow quality: Good. This was a docs-only PR by a human author (ralphbean) to strengthen breaking-change guidance across AGENTS.md, COMMITS.md, CONTRIBUTING.md, and a skill file. The review→fix→re-review cycle worked well for the most part, with one infrastructure failure.

Timeline

1. Jun 25 14:11 — Human opens PR docs: strengthen breaking-change marking guidance #2657 (closes ADR-0045 Phase 4 PR #2446 shipped breaking change without ! marker #2654)
2. Jun 25 14:14–14:24 — Review agent runs, finds 3 medium findings: internal-consistency (COMMITS.md line 3 contradicts new PR-title-based guidance), terminology-consistency (important-severity is non-standard), protected-path (AGENTS.md requires human approval)
3. Jun 25 14:50 — Human triggers /fs-fix fix the stale-docs and technical accuracy findings (addressing findings from an earlier review pass)
4. Jun 25 14:53–14:58 — Fix agent fixes 2/3 findings in ~5 min: softened GoReleaser ! claim in COMMITS.md, updated stale changelog note in cutting-releases skill. Correctly dismissed protected-path as informational.
5. Jun 25 15:02–15:32 — Re-review triggered on fix commit but failed with 422 Unprocessable Entity when submitting inline comments via GitHub API. The review completed internally but couldn't post results.
6. Jun 26 14:00 — Human reviewer (rh-hemartin) approves
7. Jun 29 17:25 — PR merged

What went well

• Review agent caught real issues. Both the technical-accuracy finding (GoReleaser doesn't actually group by !) and the stale-doc finding (skill file claimed commit-driven changelog) were genuine accuracy problems.
• Fix agent was fast and precise. 5-minute turnaround, correct fixes, appropriate dismissal of the informational finding.
• Clean handoff. The /fs-fix command with specific finding references gave the fix agent clear scope.

Gaps (all covered by existing issues)

• 422 review submission failure broke the re-review feedback loop. The post-fix review completed analysis but failed to submit. Already tracked by #2569 (diff-hunk validation edge cases) and #1067 (graceful 422 handling). This PR provides additional evidence that 422 failures have real workflow impact — the internal-consistency finding (COMMITS.md line 3 still says "every commit on main" while the rest of the file says PR titles) was never formally re-surfaced after the fix commit.
• Non-deterministic findings across review runs on the same commit. The review produced different finding sets across passes (technical-accuracy/stale-doc in one, internal-consistency/terminology-consistency in another). Already tracked by #2746.

No new proposals

All identified gaps are covered by existing open issues. No new proposals warranted.