diff --git a/.claude/skills/release/SKILL.md b/.claude/skills/release/SKILL.md index ec6a825882..60713e6910 100644 --- a/.claude/skills/release/SKILL.md +++ b/.claude/skills/release/SKILL.md @@ -72,13 +72,36 @@ Verify all of these first. If any fails, stop and say what is wrong. 4. **Working tree is clean**: `git status --porcelain`. If not, warn and ask whether to fold those changes into the release. 5. **`[Unreleased]` has content.** If empty, the release has no notes. Say so. -6. **Entries are the right shape.** They accumulate one PR at a time and drift long. +6. **Credit every entry to its pull request and its author**, in the form GitHub's + generated release notes use. Do this before anything rewords an entry: the script + reads each line's commit with `git blame`, and a line reworded in the working tree + blames to nothing and is skipped. + + ```bash + python3 scripts/ci/changelog_credits.py --dry-run | tail -5 + python3 scripts/ci/changelog_credits.py + ``` + + Each entry ends up as `(#2905 by @digows)`, or `(#1748, #2741 by @J2TeamNNL)` when it + already named an issue. The last lines name every contributor and every entry left + alone. Read that list: an entry is left bare when its commit has no pull request + number, which is a direct push. Credit it by hand from `git log` or leave it bare; + never guess a handle. + + `git blame` credits the last commit to touch a line, so an entry a maintainer reworded + in a later pull request carries the maintainer's handle. Check the contributor list + against `gh pr list --state merged --search "merged:>=" --json author` + and put a contributor's handle back on any entry that lost it. The script is + idempotent, so running it again changes only what is still bare. +7. **Entries are the right shape.** They accumulate one PR at a time and drift long. Per `CLAUDE.md` rule 1 and Keep a Changelog 1.1.0, an entry is a fragment naming - the change, one line, aiming under 120 characters: + the change, one line, aiming under 120 characters. The credit is not part of the + fragment, so measure without it: ```bash awk '/^## \[Unreleased\]/{f=1;next} /^## \[/{f=0} f' CHANGELOG.md \ - | grep '^- ' | awk '{ t+=length($0); n++; if (length($0)>120) o++ } \ + | grep '^- ' | sed -E 's/ \(#[0-9, #]+ by @[A-Za-z0-9-]+\)$//' \ + | awk '{ t+=length($0); n++; if (length($0)>120) o++ } \ END { if (!n) { print "no entries"; exit } \ print n" entries, avg "int(t/n)" chars, "o+0" over 120" }' ``` @@ -96,26 +119,27 @@ Verify all of these first. If any fails, stop and say what is wrong. If entries run over or match, rewrite the whole section before finalizing: cut each to the notable difference, turn every `X now does Y instead of Z` into the bug or the thing itself, drop trailing `so ...` clauses, merge entries describing one - change, keep every `(#1234)`. Diff the reference IDs before and after to prove none - were dropped. The explanation belongs in the PR body. At 0.67.0 this arrived with - 211 entries averaging 300 characters, the longest 1,685. -7. **On `main`**: warn, do not block. -8. **SwiftLint is clean**: `swiftlint lint --strict`. Fix what it finds first, in its + change, keep every trailing `(#1234 by @handle)` exactly as it is. Diff the + reference IDs and handles before and after to prove none were dropped. The + explanation belongs in the PR body. At 0.67.0 this arrived with 211 entries + averaging 300 characters, the longest 1,685. +8. **On `main`**: warn, do not block. +9. **SwiftLint is clean**: `swiftlint lint --strict`. Fix what it finds first, in its own commit. -9. **Report the last full-suite verdict on `main`**: warn, do not block. - - ```bash - gh run list --workflow=macos-tests.yml --branch main --limit 1 \ - --json conclusion,headSha,createdAt -q '.[] | "\(.conclusion // "in progress") \(.headSha[0:9]) \(.createdAt)"' - ``` - - Say the verdict and the commit it belongs to, then carry on. This reports rather - than blocks on purpose: `main` is red or cancelled far more often than green, on - merge skew rather than on real defects, and a hard gate with no merge queue behind - it would stop releases instead of improving them. The release tag is currently the - only unconditional full-suite run, so knowing what the last one said is worth the - one command. Eight of the last seventeen releases had their tag moved onto extra - commits before going green. +10. **Report the last full-suite verdict on `main`**: warn, do not block. + + ```bash + gh run list --workflow=macos-tests.yml --branch main --limit 1 \ + --json conclusion,headSha,createdAt -q '.[] | "\(.conclusion // "in progress") \(.headSha[0:9]) \(.createdAt)"' + ``` + + Say the verdict and the commit it belongs to, then carry on. This reports rather + than blocks on purpose: `main` is red or cancelled far more often than green, on + merge skew rather than on real defects, and a hard gate with no merge queue behind + it would stop releases instead of improving them. The release tag is currently the + only unconditional full-suite run, so knowing what the last one said is worth the + one command. Eight of the last seventeen releases had their tag moved onto extra + commits before going green. The release job re-checks what it can once the tag is pushed. It fails if the tag disagrees with `MARKETING_VERSION`, and it fails if `CURRENT_PROJECT_VERSION` did not diff --git a/.github/workflows/repo-hygiene.yml b/.github/workflows/repo-hygiene.yml index 40916825dc..9ce505474e 100644 --- a/.github/workflows/repo-hygiene.yml +++ b/.github/workflows/repo-hygiene.yml @@ -106,6 +106,9 @@ jobs: - name: Validate release note extraction run: python3 scripts/ci/test_release_notes.py + - name: Validate changelog contributor credits + run: python3 scripts/ci/test_changelog_credits.py + # Only a real release runs sign-and-appcast.sh, so nothing on a pull request ever exercised # the code that edits the file every install polls. These two cover the parts that can be # tested without a signing key: which bytes move, and which releases are refused. diff --git a/CLAUDE.md b/CLAUDE.md index b0de65e26d..c5d637b288 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -327,6 +327,8 @@ These are **non-negotiable**, never skip them: No file paths, class names, or method signatures; reference IDs go in parens at the end: `(#1234)`. Backticked type or column names are fine when they are what the user sees. Two entries describing one change get merged, not listed twice. 0.67.0 arrived with 211 entries averaging 300 characters, one of them 1,685, and had to be rewritten wholesale at release time. + **Every released entry credits its pull request and its author**, the way GitHub's generated release notes do: `(#2905 by @digows)`, or `(#1748, #2741 by @J2TeamNNL)` when the entry names an issue, which stays first. The author is whoever opened the pull request, never whoever merged it. Do not write the credit by hand in a pull request: the number does not exist until the pull request is opened, and a contributor should not need a second commit for it. The release stamps it with `scripts/ci/changelog_credits.py`, which reads each entry's commit with `git blame`, the pull request number from the squash subject and the author from `gh pr view`. Blame credits the last commit to touch a line, so reword a contributor's entry in their pull request, never in a later cleanup that takes the credit from them. + 2. **Localization**: Use `String(localized:)` for new user-facing strings in computed properties, AppKit code, alerts, and error descriptions. SwiftUI view literals (`Text("literal")`, `Button("literal")`) auto-localize. Do NOT localize technical terms (font names, database types, SQL keywords, encoding names). Never use `String(localized:)` with string interpolation, `String(localized: "Preview \(name)")` creates a dynamic key that never matches the strings catalog. Use `String(format: String(localized: "Preview %@"), name)`. 3. **Documentation**: Update docs in `docs/` (Mintlify-based) when adding/changing features: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f443df67d8..4bf60183d5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -94,7 +94,7 @@ One logical change per PR. Make sure tests pass and lint is clean. Checklist: - [ ] Tests added or updated -- [ ] `CHANGELOG.md` updated under `[Unreleased]` (skip for unreleased-only fixes) +- [ ] `CHANGELOG.md` updated under `[Unreleased]` (skip for unreleased-only fixes). Leave the credit off: the release adds `(#123 by @you)` to every entry from your pull request, so there is no number to guess and no second commit to push - [ ] Docs updated in `docs/` if the change affects user-facing behavior - [ ] User-facing strings localized - [ ] No SwiftLint/SwiftFormat violations diff --git a/scripts/ci/changelog_credits.py b/scripts/ci/changelog_credits.py new file mode 100644 index 0000000000..4119a83455 --- /dev/null +++ b/scripts/ci/changelog_credits.py @@ -0,0 +1,186 @@ +#!/usr/bin/env python3 +"""Credit every CHANGELOG entry of one version to the pull request and the person that added it. + +GitHub's generated release notes end each line with `by @author in #123`. The CHANGELOG is what +the release body is built from, so it carries the same credit, in the parens every entry already +ends with: `(#123 by @author)`, or `(#1748, #2741 by @author)` when the entry names an issue. + +An entry cannot carry this when it is written: the pull request has no number until it is opened, +and a contributor should not have to push a second commit to add one. So the release stamps it. +The credit comes from the history, not from anyone's memory: + +1. `git blame` names the commit that wrote each entry line. +2. A squash merge ends its subject with the pull request number, `(#123)`. +3. `gh pr view` names that pull request's author, which is the person credited, never whoever + merged it. + +Blame reports the last commit to touch a line, so a maintainer who rewords a contributor's entry +takes the credit for it. Reword in the contributor's own pull request, or restore the credit by +hand. An entry whose commit carries no pull request number (a direct push) is left alone and +listed, and so is a working-tree line that is not committed yet. + +Usage: + python3 scripts/ci/changelog_credits.py [--section Unreleased] [--dry-run] + +Run it from the repository root. It rewrites CHANGELOG.md in place and is idempotent: an entry +that already ends in a credit is never touched again. +""" + +import argparse +import json +import re +import subprocess +import sys +from dataclasses import dataclass +from pathlib import Path + +REFERENCES = re.compile(r"\s*\((#\d+(?:,\s*#\d+)*)\)\s*$") +CREDITED = re.compile(r"\(#\d+(?:,\s*#\d+)* by @[A-Za-z0-9][A-Za-z0-9-]*\)\s*$") +PULL_REQUEST_SUBJECT = re.compile(r"\(#(\d+)\)\s*$") +UNCOMMITTED = "0" * 40 + + +def is_entry(line): + return line.startswith("- ") + + +def is_credited(line): + return CREDITED.search(line) is not None + + +def normalized_login(login): + """`gh` reports a GitHub App as `app/name`, which GitHub itself renders as `@name`.""" + return login.split("/", 1)[1] if login.startswith("app/") else login + + +def credit_entry(line, pull_request, author): + """`line` with `(#pull_request by @author)` folded into the reference parens it ends with.""" + if is_credited(line): + return line + body = line.rstrip() + references = [] + existing = REFERENCES.search(body) + if existing: + references = [reference.strip() for reference in existing.group(1).split(",")] + body = body[:existing.start()] + if f"#{pull_request}" not in references: + references.append(f"#{pull_request}") + return f"{body} ({', '.join(references)} by @{normalized_login(author)})" + + +def section_bounds(lines, section): + """Zero-based `[start, end)` of the entries under `## [section]`, heading excluded.""" + heading = f"## [{section}]" + start = next((index for index, line in enumerate(lines) if line.startswith(heading)), None) + if start is None: + raise ValueError(f"CHANGELOG.md has no {heading} section") + end = next( + (index for index in range(start + 1, len(lines)) if lines[index].startswith("## [")), + len(lines), + ) + return start + 1, end + + +@dataclass +class BlamedLine: + sha: str + summary: str + + +def parse_blame(porcelain): + """One `BlamedLine` per line of `git blame --line-porcelain` output, in order.""" + blamed = [] + sha = None + summary = "" + for line in porcelain.split("\n"): + header = re.match(r"^([0-9a-f]{40}) \d+ \d+", line) + if header: + sha = header.group(1) + summary = "" + elif line.startswith("summary "): + summary = line[len("summary "):] + elif line.startswith("\t") and sha is not None: + blamed.append(BlamedLine(sha=sha, summary=summary)) + return blamed + + +def pull_request_of(blamed): + if blamed.sha == UNCOMMITTED: + return None + match = PULL_REQUEST_SUBJECT.search(blamed.summary) + return match.group(1) if match else None + + +def blame_section(start, end): + result = subprocess.run( + ["git", "blame", "--line-porcelain", "-L", f"{start + 1},{end}", "--", "CHANGELOG.md"], + capture_output=True, + text=True, + check=True, + ) + return parse_blame(result.stdout) + + +def author_of(pull_request, cache): + if pull_request not in cache: + result = subprocess.run( + ["gh", "pr", "view", pull_request, "--json", "author"], + capture_output=True, + text=True, + check=True, + ) + cache[pull_request] = json.loads(result.stdout)["author"]["login"] + return cache[pull_request] + + +def main(argv=None): + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument("--section", default="Unreleased", help="the version heading to credit") + parser.add_argument("--dry-run", action="store_true", help="print the result instead of writing it") + args = parser.parse_args(argv) + + path = Path("CHANGELOG.md") + lines = path.read_text(encoding="utf-8").split("\n") + try: + start, end = section_bounds(lines, args.section) + except ValueError as error: + sys.exit(f"ERROR: {error}") + if start == end: + print(f"[{args.section}] has no entries") + return 0 + + blamed = blame_section(start, end) + if len(blamed) != end - start: + sys.exit("ERROR: git blame did not return one record per line") + + authors = {} + credited = 0 + skipped = [] + for offset, record in enumerate(blamed): + index = start + offset + line = lines[index] + if not is_entry(line) or is_credited(line): + continue + pull_request = pull_request_of(record) + if pull_request is None: + reason = "not committed yet" if record.sha == UNCOMMITTED else f"{record.sha[:9]} has no pull request number" + skipped.append(f"line {index + 1}: {reason}: {line[:90]}") + continue + lines[index] = credit_entry(line, pull_request, author_of(pull_request, authors)) + credited += 1 + + if args.dry_run: + print("\n".join(lines[start:end])) + else: + path.write_text("\n".join(lines), encoding="utf-8") + + contributors = sorted({normalized_login(login) for login in authors.values()}) + print(f"Credited {credited} entries in [{args.section}] across {len(authors)} pull requests.") + print(f"Contributors: {', '.join('@' + login for login in contributors) or 'none'}") + for line in skipped: + print(f"Left uncredited, {line}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/ci/test_changelog_credits.py b/scripts/ci/test_changelog_credits.py new file mode 100644 index 0000000000..0a8f9c89ab --- /dev/null +++ b/scripts/ci/test_changelog_credits.py @@ -0,0 +1,128 @@ +#!/usr/bin/env python3 +"""Tests for the contributor credit the release stamps on CHANGELOG entries. + +Run: python3 scripts/ci/test_changelog_credits.py +""" + +import unittest + +import changelog_credits as credits + + +SHA_A = "a" * 40 +SHA_B = "b" * 40 + + +class CreditEntryTests(unittest.TestCase): + def test_an_entry_with_no_reference_gains_the_pull_request_and_its_author(self): + self.assertEqual( + credits.credit_entry("- Two chevrons on the Tags row.", "2919", "datlechin"), + "- Two chevrons on the Tags row. (#2919 by @datlechin)", + ) + + def test_an_issue_reference_stays_first_and_the_pull_request_joins_it(self): + self.assertEqual( + credits.credit_entry("- OceanBase connection type. (#1748)", "2741", "J2TeamNNL"), + "- OceanBase connection type. (#1748, #2741 by @J2TeamNNL)", + ) + + def test_a_reference_that_is_already_the_pull_request_is_not_repeated(self): + self.assertEqual( + credits.credit_entry("- Crash on launch. (#2930)", "2930", "datlechin"), + "- Crash on launch. (#2930 by @datlechin)", + ) + + def test_several_references_keep_their_order(self): + self.assertEqual( + credits.credit_entry("- Spanner plugin. (#1226, #2480)", "2481", "datlechin"), + "- Spanner plugin. (#1226, #2480, #2481 by @datlechin)", + ) + + def test_a_trailing_parenthetical_that_is_not_a_reference_is_left_as_prose(self): + self.assertEqual( + credits.credit_entry("- Empty object copied as an array. (Copy Objects)", "2926", "datlechin"), + "- Empty object copied as an array. (Copy Objects) (#2926 by @datlechin)", + ) + + def test_crediting_twice_changes_nothing(self): + once = credits.credit_entry("- Nested fields shown as null.", "2905", "digows") + self.assertEqual(credits.credit_entry(once, "2905", "digows"), once) + + def test_a_github_app_is_credited_by_its_name(self): + self.assertEqual( + credits.credit_entry("- Bumped a dependency.", "12", "app/dependabot"), + "- Bumped a dependency. (#12 by @dependabot)", + ) + + def test_trailing_whitespace_does_not_hide_the_references(self): + self.assertEqual( + credits.credit_entry("- Stale grid. (#100) ", "101", "filipac"), + "- Stale grid. (#100, #101 by @filipac)", + ) + + +class SectionTests(unittest.TestCase): + CHANGELOG = [ + "# Changelog", + "", + "## [Unreleased]", + "", + "### Fixed", + "", + "- One.", + "", + "## [1.0.0] - 2026-01-01", + "", + "- Old.", + ] + + def test_the_section_runs_to_the_next_version_heading(self): + self.assertEqual(credits.section_bounds(self.CHANGELOG, "Unreleased"), (3, 8)) + + def test_the_last_section_runs_to_the_end_of_the_file(self): + self.assertEqual(credits.section_bounds(self.CHANGELOG, "1.0.0"), (9, 11)) + + def test_a_missing_section_is_an_error(self): + with self.assertRaises(ValueError): + credits.section_bounds(self.CHANGELOG, "9.9.9") + + +class BlameTests(unittest.TestCase): + PORCELAIN = "\n".join([ + f"{SHA_A} 7 7 1", + "author Someone", + "summary fix(editor): keep the caret (#2915)", + "filename CHANGELOG.md", + "\t- One.", + f"{SHA_B} 8 8 1", + "author Someone", + "summary chore: tidy the changelog", + "filename CHANGELOG.md", + "\t- Two.", + f"{credits.UNCOMMITTED} 9 9 1", + "author Not Committed Yet", + "summary Version CHANGELOG.md", + "filename CHANGELOG.md", + "\t- Three.", + "", + ]) + + def test_each_line_keeps_the_commit_that_wrote_it(self): + blamed = credits.parse_blame(self.PORCELAIN) + self.assertEqual([record.sha for record in blamed], [SHA_A, SHA_B, credits.UNCOMMITTED]) + + def test_a_squash_merge_names_its_pull_request(self): + blamed = credits.parse_blame(self.PORCELAIN) + self.assertEqual(credits.pull_request_of(blamed[0]), "2915") + + def test_a_direct_push_names_no_pull_request(self): + blamed = credits.parse_blame(self.PORCELAIN) + self.assertIsNone(credits.pull_request_of(blamed[1])) + + def test_an_uncommitted_line_names_no_pull_request(self): + blamed = credits.parse_blame(self.PORCELAIN) + self.assertIsNone(credits.pull_request_of(blamed[2])) + + +if __name__ == "__main__": + unittest.main()