diff --git a/.claude/skills/bump-ark/README.md b/.claude/skills/bump-ark/README.md index 10bc17139f25..82755a09958e 100644 --- a/.claude/skills/bump-ark/README.md +++ b/.claude/skills/bump-ark/README.md @@ -10,7 +10,7 @@ The script creates a Positron-side PR that bumps the submodule ref, and fills th - A "Closes #XXX" line for every issue mentioned in the release notes, and every "Addresses #XXX" mention in the Ark PRs. -- A commit section listing every commit since the last bump. The commits are gathered with a `--first-parent` walk, so that only squash merges, merge commits, and commits directly pushed to main are included. +- A commit section listing every commit since the last bump. The commits are gathered with a `--first-parent` walk, so that only squash merges, merge commits, and commits directly pushed to main are included. Commits belonging to an unmerged PR collapse to one line per PR, in the squash-merge form they will take once that PR lands. #### Usage @@ -32,6 +32,8 @@ You call the script with the PR number to open a Positron PR and run CI tests. I Make sure to include the relevant release notes in your Ark PR so the script can generate up-to-date release notes in the Positron PR. +While your PR is open, its commits render as one collapsed line in the commit section, because that's what Ark's log will show once the PR squash-merges. Stacked PRs are handled too: the script matches each PR's base branch against the head branch of another open PR, walking down to the PR based on Ark main, then collapses each PR in the stack to its own line and pulls in its release notes and `Closes` references. So a PR stacked four deep gives you four lines instead of a run of individual commits, and the release notes cover the whole stack rather than the top PR alone. Detection is based on base branches only, so it works whether or not you use `gh stack`. + When ready, merge your branch to main and invoke the script one last time. The Positron side PR will now point to the merge commit on main. This works no matter the merge type (squash or branch merge). If there is other work committed to main, the script will find the release notes of the corresponding PRs. That's on purpose: If your submodule bump includes other people's work, the entire set of changes should be documented here. That's often the sign that a concurrent bump is going on and you might want to coordinate in that case. If the other work gets merged in, call the script/skill again to update the release notes accordingly. diff --git a/.claude/skills/bump-ark/SKILL.md b/.claude/skills/bump-ark/SKILL.md index a19688353fac..fc7a0b000eb9 100644 --- a/.claude/skills/bump-ark/SKILL.md +++ b/.claude/skills/bump-ark/SKILL.md @@ -37,10 +37,15 @@ alike, since GitHub records `merge_commit_sha` for all three. When the bumped PRs close Positron issues, the body opens with `Closes #` lines. Then the tag line, a `### Release Notes` section aggregated from the bumped Ark PRs, and a `### Commits` first-parent list of the Ark commits between -the current submodule pointer and the target. +the current submodule pointer and the target. Commits belonging to an unmerged +PR collapse to one line per PR, in the squash-merge form they will take once that +PR lands. The target's stack is found by matching each PR's base branch against +the head branch of another open PR, down to the PR based on Ark main, so a +stacked target contributes one line per PR in the stack. Release notes are scraped from each bumped Ark PR's `#### New Features` / -`#### Bug Fixes` bullets. `parse_description.py` is a vendored copy of +`#### Bug Fixes` bullets, covering every PR in the target's stack rather than the +target alone. `parse_description.py` is a vendored copy of `posit-dev/positron-release-notes`'s parser, so extraction matches the release-notes collector; `bump_notes.py` is the skill-local glue on top of it (section rendering, `Closes` collection). diff --git a/.claude/skills/bump-ark/bump_ark.py b/.claude/skills/bump-ark/bump_ark.py index 93903db958d2..591bd50c7c3c 100644 --- a/.claude/skills/bump-ark/bump_ark.py +++ b/.claude/skills/bump-ark/bump_ark.py @@ -32,6 +32,7 @@ --dry-run print the PR body to stdout; no branch, ref, or PR is touched""" ARK_REPO = "posit-dev/ark" +ARK_BASE_BRANCH = "main" POSITRON_REPO = "posit-dev/positron" SUBMODULE_PATH = "extensions/positron-r/ark" BASE_BRANCH = "main" @@ -112,9 +113,14 @@ def build_bump( walk = first_parent_commits( compare.merge_base, resolution.sha, compare.commit_map, compare.total_commits ) - commit_lines = "\n".join(commit_line(sha, subject) for sha, subject in walk) - notes = collect_release_notes(walk, resolution.open_pr_number) + associations = pr_associations(walk, resolution.target_pr) + chain, chain_messages = stack_chain(resolution.target_pr, associations.unmerged) + for message in chain_messages: + eprint(message) + commit_lines = walk_lines(group_walk(walk, associations, chain)) + + notes = collect_release_notes(associations, chain) closes = "\n".join(f"Closes #{n}" for n in notes["closes"]) body = build_body(closes, tag_line(tag_args), notes["notes"], commit_lines) @@ -155,25 +161,47 @@ def check_gh(): # --- target resolution ------------------------------------------------------ +@dataclass +class UnmergedPr: + number: int + title: str + body: str + base_ref: str + head_ref: str + + @dataclass class Resolution: sha: str title: str branch: str - # Set only on an open PR bump: its head commit belongs to the still-open PR, - # which the walk's merged-only filter drops, so its notes are added by number - # instead. None on a merged PR bump and a main bump, whose notes ride the walk. - open_pr_number: Optional[str] + # Set only on an unmerged PR bump. The target's own commits, and those of the + # PRs it is stacked on, belong to no merged PR, so the walk's merged-only + # filter can't see them. None on a merged PR bump and a main bump, whose + # notes ride the walk. + target_pr: Optional[UnmergedPr] is_pr_bump: bool +def unmerged_pr(pr: dict) -> UnmergedPr: + base = pr.get("base") or {} + head = pr.get("head") or {} + return UnmergedPr( + number=pr["number"], + title=pr.get("title") or "", + body=pr.get("body") or "", + base_ref=base.get("ref") or "", + head_ref=head.get("ref") or "", + ) + + # Each logical bump gets a single fixed branch it advances in place. A PR bump # uses `bump-ark/pr-` and tracks one Ark PR across its whole life: while the # PR is open it targets the PR head, once merged it targets the merge commit. A # main bump uses `bump-ark/main` and tracks the latest Ark main. def resolve_target(arg: str) -> Resolution: if arg == "main": - sha = gh_json("api", f"repos/{ARK_REPO}/commits/main")["sha"] + sha = gh_json("api", f"repos/{ARK_REPO}/commits/{ARK_BASE_BRANCH}")["sha"] return Resolution(sha, "Bump Ark to latest main", "bump-ark/main", None, False) if not arg.isdigit(): @@ -226,7 +254,7 @@ def pr_resolution(pr_number: str, pr: dict) -> tuple[Resolution, list[str]]: " The submodule can't resolve to a fork commit, so refusing to bump." ) - return Resolution(head["sha"], title, branch, pr_number, True), messages + return Resolution(head["sha"], title, branch, unmerged_pr(pr), True), messages # --- ancestry --------------------------------------------------------------- @@ -394,30 +422,148 @@ def walk_first_parents( return walk -# --- release notes ---------------------------------------------------------- +# --- PR associations and stacking ------------------------------------------- -# Gather the merged Ark PRs behind the walked commits, plus the target PR itself -# on an open PR bump, and hand their bodies to the vendored release-notes parser. +@dataclass +class PrAssociations: + # PR number -> body, for the merged PRs behind the walked commits. + merged_bodies: dict[int, str] + # PR number -> PR, for the unmerged PRs whose branch contains a walked commit. + unmerged: dict[int, UnmergedPr] + # Walked commit sha -> the unmerged PRs GitHub associates with it. + commit_prs: dict[str, list[int]] + + +@dataclass +class GroupEntry: + # None for a commit rendered on its own, otherwise the PR whose commits this + # entry collapses. + pr_number: Optional[int] + # The entry's own commit, or the newest of the group. + sha: str + subject: str + count: int + title: str = "" + + +# Fetch, for every walked commit, every PR whose branch contains it. Splits them +# into merged PR bodies and unmerged PRs, plus the per-commit association lists +# `group_walk` needs to attribute a commit to the right PR in a stack. # -# On an open PR bump the target PR is added explicitly: its head commit is -# associated only with the still-open PR, which the `merged_at` filter drops, yet -# its notes are the point of the bump. On a merged PR bump `open_pr_number` is -# empty, because the merge commit already surfaces the PR through the walk. The -# dict keeps the first body seen per PR number, collapsing a PR that spans several -# walked commits (or the target reappearing among a commit's associated PRs). -def collect_release_notes( - walk: list[tuple[str, str]], open_pr_number: Optional[str] -) -> dict: - bodies: dict[int, str] = {} +# `target_pr` seeds `unmerged` before the walk, so an unmerged bump target that +# GitHub's association list happens to omit still roots its stack. +def pr_associations( + walk: list[tuple[str, str]], target_pr: Optional[UnmergedPr] +) -> PrAssociations: + merged_bodies: dict[int, str] = {} + unmerged: dict[int, UnmergedPr] = {} + commit_prs: dict[str, list[int]] = {} + + if target_pr is not None: + unmerged[target_pr.number] = target_pr + for sha, _ in walk: for pr in gh_json("api", f"repos/{ARK_REPO}/commits/{sha}/pulls") or []: + number = pr["number"] if pr.get("merged_at"): - bodies.setdefault(pr["number"], pr.get("body") or "") + merged_bodies.setdefault(number, pr.get("body") or "") + else: + unmerged.setdefault(number, unmerged_pr(pr)) + commit_prs.setdefault(sha, []).append(number) + + return PrAssociations(merged_bodies, unmerged, commit_prs) - if open_pr_number: - pr = gh_json("api", f"repos/{ARK_REPO}/pulls/{open_pr_number}") - bodies.setdefault(pr["number"], pr.get("body") or "") + +# The target's stack, top first, ending at the PR based on Ark main, plus the +# stderr messages the caller should emit. Each step matches a PR's base branch +# against the head branch of another unmerged PR, so it recognizes a stack however +# it was built. `target_pr` is None on a merged PR bump and a main bump, neither of +# which has a stack, so they get back `([], [])`. +# +# The chain stops at a base branch no unmerged PR heads, which is silent because +# that also covers the ordinary case of a branch pushed without a PR. The commits +# below such a branch stay listed individually. +def stack_chain( + target_pr: Optional[UnmergedPr], unmerged: dict[int, UnmergedPr] +) -> tuple[list[int], list[str]]: + if target_pr is None: + return [], [] + + chain = [target_pr.number] + current = target_pr + + while current.base_ref != ARK_BASE_BRANCH: + next_pr = next( + ( + pr + for pr in unmerged.values() + if pr.head_ref == current.base_ref and pr.number not in chain + ), + None, + ) + if next_pr is None: + break + chain.append(next_pr.number) + current = next_pr + + messages = [] + if len(chain) > 1: + stacked_on = ", ".join(f"#{number}" for number in chain[1:]) + messages.append( + f"PR #{chain[0]} is stacked on {stacked_on}. Collapsing each PR's commits into one line." + ) + + return chain, messages + + +# Group the walk into one entry per `chain` PR, collapsing its commits, and one +# entry per commit belonging to no `chain` PR. A commit's owner is the `chain` PR +# closest to Ark main among those associated with it, because every PR's branch +# contains the commits of the PRs below it, so a bottom-of-stack commit is listed +# under the whole stack above it too. Unmerged PRs outside `chain` are ignored, +# which keeps a mid-stack target from pulling in the PRs stacked above it. +def group_walk( + walk: list[tuple[str, str]], associations: PrAssociations, chain: list[int] +) -> list[GroupEntry]: + chain_index = {number: index for index, number in enumerate(chain)} + entries: list[GroupEntry] = [] + entry_by_pr: dict[int, GroupEntry] = {} + + for sha, subject in walk: + candidates = [ + n for n in associations.commit_prs.get(sha, []) if n in chain_index + ] + owner = max(candidates, key=lambda n: chain_index[n]) if candidates else None + + if owner is None: + entries.append(GroupEntry(None, sha, subject, 1)) + continue + + entry = entry_by_pr.get(owner) + if entry is None: + entry = GroupEntry( + owner, sha, subject, 1, associations.unmerged[owner].title + ) + entry_by_pr[owner] = entry + entries.append(entry) + else: + entry.count += 1 + + return entries + + +# --- release notes ---------------------------------------------------------- + + +# Gather the release notes of every merged PR behind the walked commits and of +# every PR in the target's stack, and hand their bodies to the vendored +# release-notes parser. `chain` order (top first) matches the newest-first +# ordering the rest of the body uses. +def collect_release_notes(associations: PrAssociations, chain: list[int]) -> dict: + bodies = dict(associations.merged_bodies) + for number in chain: + bodies.setdefault(number, associations.unmerged[number].body) return build_notes([{"number": n, "body": b} for n, b in bodies.items()]) @@ -436,14 +582,39 @@ def tag_line(tags: list[str]) -> str: return " ".join(result) +# Escape brackets in link text, so a stray `[`/`]` (e.g. `[skip ci]`) can't break +# the surrounding markdown link syntax. +def escape_brackets(text: str) -> str: + return text.replace("[", r"\[").replace("]", r"\]") + + # Render a walked commit as a markdown link to the Ark commit. The link does # double duty: it points at the Ark side, and it stops GitHub from autolinking the # `(#NNNN)` Ark PR reference in the subject to a Positron issue of the same number, -# since autolinking doesn't fire inside link text. Brackets in the subject are -# escaped so a stray `[`/`]` (e.g. `[skip ci]`) can't break the link syntax. +# since autolinking doesn't fire inside link text. def commit_line(sha: str, subject: str) -> str: - text = subject.replace("[", r"\[").replace("]", r"\]") - return f"- [{text}](https://github.com/{ARK_REPO}/commit/{sha})" + return f"- [{escape_brackets(subject)}](https://github.com/{ARK_REPO}/commit/{sha})" + + +# Render an unmerged PR's collapsed group as a markdown link to the PR, the +# squash-merge-style line it will get once the PR merges. Same link-text +# placement of `(#N)` as `commit_line`, for the same autolinking reason. +def pr_line(number: int, title: str, count: int) -> str: + text = escape_brackets(f"{title} (#{number})") + unit = "commit" if count == 1 else "commits" + return f"- [{text}](https://github.com/{ARK_REPO}/pull/{number}) ({count} unmerged {unit})" + + +# Render the `### Commits` block: a `pr_line` for an entry with a `pr_number`, a +# `commit_line` otherwise. +def walk_lines(entries: list[GroupEntry]) -> str: + lines = [] + for entry in entries: + if entry.pr_number is None: + lines.append(commit_line(entry.sha, entry.subject)) + else: + lines.append(pr_line(entry.pr_number, entry.title, entry.count)) + return "\n".join(lines) # Assemble the PR body. `Closes` lines go first (GitHub reads closing keywords diff --git a/.claude/skills/bump-ark/test_bump_ark.py b/.claude/skills/bump-ark/test_bump_ark.py index 44474ade4d1a..91dfd9148333 100644 --- a/.claude/skills/bump-ark/test_bump_ark.py +++ b/.claude/skills/bump-ark/test_bump_ark.py @@ -18,14 +18,20 @@ from bump_ark import ( # noqa: E402 ARK_REPO, BumpError, + GroupEntry, + PrAssociations, + UnmergedPr, blocked_by_pr_owner, build_body, classify_ancestry, commit_line, commit_summary, first_parent_commits, + group_walk, parse_args, + pr_line, pr_resolution, + stack_chain, tag_line, walk_first_parents, ) @@ -34,6 +40,10 @@ class DecidePrResolutionTest(unittest.TestCase): def open_pr(self, **overrides): pr = { + "number": 42, + "title": "Some PR title", + "body": "some body", + "base": {"ref": "main"}, "head": {"sha": "headsha", "repo": {"full_name": ARK_REPO}}, "state": "open", "merged_at": None, @@ -42,16 +52,19 @@ def open_pr(self, **overrides): pr.update(overrides) return pr - def test_open_pr_targets_head_and_sets_open_pr_number(self): + def test_open_pr_targets_head_and_sets_target_pr(self): resolution, messages = pr_resolution("42", self.open_pr()) self.assertEqual(resolution.sha, "headsha") self.assertEqual(resolution.title, "Bump Ark to posit-dev/ark#42") self.assertEqual(resolution.branch, "bump-ark/pr-42") - self.assertEqual(resolution.open_pr_number, "42") + self.assertEqual( + resolution.target_pr, + UnmergedPr(42, "Some PR title", "some body", "main", ""), + ) self.assertTrue(resolution.is_pr_bump) self.assertEqual(messages, []) - def test_merged_pr_targets_merge_commit_and_clears_open_pr_number(self): + def test_merged_pr_targets_merge_commit_and_clears_target_pr(self): pr = self.open_pr( merged_at="2026-01-01T00:00:00Z", merge_commit_sha="mergesha", @@ -59,7 +72,7 @@ def test_merged_pr_targets_merge_commit_and_clears_open_pr_number(self): ) resolution, messages = pr_resolution("42", pr) self.assertEqual(resolution.sha, "mergesha") - self.assertIsNone(resolution.open_pr_number) + self.assertIsNone(resolution.target_pr) self.assertTrue(resolution.is_pr_bump) self.assertEqual( messages, @@ -89,7 +102,7 @@ def test_merged_pr_without_merge_commit_is_fatal(self): def test_closed_unmerged_pr_warns_but_targets_head(self): resolution, messages = pr_resolution("42", self.open_pr(state="closed")) self.assertEqual(resolution.sha, "headsha") - self.assertEqual(resolution.open_pr_number, "42") + self.assertEqual(resolution.target_pr.number, 42) self.assertEqual(len(messages), 1) self.assertIn("is closed (not merged)", messages[0]) @@ -277,7 +290,138 @@ def test_pr_reference_stays_inside_the_link_text(self): def test_escapes_brackets_in_subject(self): line = commit_line("abc123", "Tweak CI [skip ci]") self.assertEqual( - line, r"- [Tweak CI \[skip ci\]](https://github.com/" + ARK_REPO + "/commit/abc123)" + line, + r"- [Tweak CI \[skip ci\]](https://github.com/" + + ARK_REPO + + "/commit/abc123)", + ) + + +class PrLineTest(unittest.TestCase): + def test_links_to_the_pr(self): + line = pr_line(1388, "Add support for Shiny app auto-loading", 12) + self.assertEqual( + line, + "- [Add support for Shiny app auto-loading (#1388)]" + f"(https://github.com/{ARK_REPO}/pull/1388) (12 unmerged commits)", + ) + + def test_pr_reference_stays_inside_the_link_text(self): + line = pr_line(42, "Fix the thing", 3) + self.assertIn("[Fix the thing (#42)]", line) + + def test_singular_commit(self): + line = pr_line(42, "Fix the thing", 1) + self.assertIn("(1 unmerged commit)", line) + self.assertNotIn("commits)", line) + + def test_escapes_brackets_in_title(self): + line = pr_line(42, "Tweak CI [skip ci]", 2) + self.assertIn(r"Tweak CI \[skip ci\] (#42)", line) + + +class StackChainTest(unittest.TestCase): + def pr(self, number, base_ref, head_ref): + return UnmergedPr(number, f"PR {number}", "", base_ref, head_ref) + + def test_none_target_yields_empty(self): + self.assertEqual(stack_chain(None, {}), ([], [])) + + def test_four_pr_stack_resolves_top_first(self): + target = self.pr(1388, "branch-1384", "branch-1388") + unmerged = { + 1388: target, + 1384: self.pr(1384, "branch-1383", "branch-1384"), + 1383: self.pr(1383, "branch-1382", "branch-1383"), + 1382: self.pr(1382, "main", "branch-1382"), + } + chain, messages = stack_chain(target, unmerged) + self.assertEqual(chain, [1388, 1384, 1383, 1382]) + self.assertEqual(len(messages), 1) + self.assertIn("#1388 is stacked on #1384, #1383, #1382", messages[0]) + + def test_lone_pr_based_on_main_yields_itself(self): + target = self.pr(42, "main", "branch-42") + chain, messages = stack_chain(target, {42: target}) + self.assertEqual(chain, [42]) + self.assertEqual(messages, []) + + def test_missing_link_stops_chain_silently(self): + # A base branch with no unmerged PR of its own is also what an ordinary + # branch pushed without a PR looks like, so it is not worth a warning. + target = self.pr(1388, "branch-missing", "branch-1388") + chain, messages = stack_chain(target, {1388: target}) + self.assertEqual(chain, [1388]) + self.assertEqual(messages, []) + + def test_ref_cycle_terminates(self): + # A's base is B's head and B's base is A's head, the chain must not loop. + a = self.pr(1, "branch-b", "branch-a") + b = self.pr(2, "branch-a", "branch-b") + chain, _ = stack_chain(a, {1: a, 2: b}) + self.assertEqual(chain, [1, 2]) + + +class GroupWalkTest(unittest.TestCase): + def test_collapses_pr_commits_into_one_entry(self): + walk = [("c2", "subject c2"), ("c1", "subject c1")] + associations = PrAssociations( + merged_bodies={}, + unmerged={1: UnmergedPr(1, "PR title", "", "main", "branch-1")}, + commit_prs={"c2": [1], "c1": [1]}, + ) + entries = group_walk(walk, associations, [1]) + self.assertEqual(entries, [GroupEntry(1, "c2", "subject c2", 2, "PR title")]) + + def test_interleaves_plain_commits(self): + walk = [("c3", "s3"), ("c2", "s2"), ("c1", "s1")] + associations = PrAssociations( + merged_bodies={}, + unmerged={1: UnmergedPr(1, "PR title", "", "main", "branch-1")}, + commit_prs={"c2": [1]}, + ) + entries = group_walk(walk, associations, [1]) + self.assertEqual( + entries, + [ + GroupEntry(None, "c3", "s3", 1), + GroupEntry(1, "c2", "s2", 1, "PR title"), + GroupEntry(None, "c1", "s1", 1), + ], + ) + + def test_attributes_shared_commit_to_lowest_chain_pr(self): + # A commit at the bottom of a stack lists every PR above it too; only the + # PR closest to Ark main (the last entry in `chain`) is the real owner. + walk = [("c1", "s1")] + associations = PrAssociations( + merged_bodies={}, + unmerged={ + 1388: UnmergedPr(1388, "Top", "", "b1384", "b1388"), + 1382: UnmergedPr(1382, "Bottom", "", "main", "b1382"), + }, + commit_prs={"c1": [1388, 1382]}, + ) + entries = group_walk(walk, associations, [1388, 1382]) + self.assertEqual(entries, [GroupEntry(1382, "c1", "s1", 1, "Bottom")]) + + def test_ignores_unmerged_pr_outside_chain(self): + walk = [("c1", "s1")] + associations = PrAssociations( + merged_bodies={}, + unmerged={99: UnmergedPr(99, "Outside", "", "main", "b99")}, + commit_prs={"c1": [99]}, + ) + entries = group_walk(walk, associations, []) + self.assertEqual(entries, [GroupEntry(None, "c1", "s1", 1)]) + + def test_empty_chain_leaves_every_commit_individual(self): + walk = [("c2", "s2"), ("c1", "s1")] + associations = PrAssociations({}, {}, {}) + entries = group_walk(walk, associations, []) + self.assertEqual( + entries, + [GroupEntry(None, "c2", "s2", 1), GroupEntry(None, "c1", "s1", 1)], ) diff --git a/extensions/positron-r/ark b/extensions/positron-r/ark index 885fac431b43..d90873cc9ab1 160000 --- a/extensions/positron-r/ark +++ b/extensions/positron-r/ark @@ -1 +1 @@ -Subproject commit 885fac431b43323836d0f6e4a68bcaa8e9c17dbd +Subproject commit d90873cc9ab129415b46e00217f97df18a39795e