From ade22656ec45d0fd137790a150229666060178e2 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 8 Jul 2026 23:06:08 -0700 Subject: [PATCH 1/3] ci(stack-breadcrumb): bring forward the evolved topology-aware breadcrumb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We sent an early stack-breadcrumb workflow to the taskless/taskless team; they evolved it with a series of fixes. This brings that version (the zero-dependency script + its tests + the workflow) back into this repo, reformatted to our Prettier style and MIT-licensed with permission. Notable improvements over our current version: - Topology-aware markers: the stack marker now encodes each member's parent (`pr=82,83:82,84:83`) instead of a flat list, so the tree shape is durable. - Additive tree: a PR stays in the breadcrumb after it merges, rendered `✅ merged` (vs `⛔ closed`), instead of vanishing. - Recorded-parent resolution: when a parent merges, GitHub deletes its branch and retargets the child onto the default branch, severing the live base/head edge. The recorded parent survives that, so the merged ancestor stays in the tree rather than disappearing. - Defensive cycle guard in the region renderer; legacy flat markers self-migrate to the recorded form on the next reconcile. The logic is repo-agnostic (no taskless/taskless-specific wiring). All 54 unit tests pass via `node --test`; CI already runs `.github/scripts/*.test.cjs`. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/stack-breadcrumb.cjs | 286 +++++++++++++++++++--- .github/scripts/stack-breadcrumb.test.cjs | 171 ++++++++++++- .github/workflows/stack-breadcrumb.yml | 7 + 3 files changed, 413 insertions(+), 51 deletions(-) diff --git a/.github/scripts/stack-breadcrumb.cjs b/.github/scripts/stack-breadcrumb.cjs index 95c0fd8d..e2dc9414 100644 --- a/.github/scripts/stack-breadcrumb.cjs +++ b/.github/scripts/stack-breadcrumb.cjs @@ -1,3 +1,7 @@ +// SPDX-License-Identifier: MIT +// Adapted from the taskless/taskless stack-breadcrumb implementation +// (@taskless/stack-breadcrumb) and brought into this repository under its MIT +// license, with permission. "use strict"; /** @@ -31,13 +35,23 @@ /** Matches the whole region, opening marker through ``. */ const REGION_PATTERN = /[\S\s]*?/; -/** Matches just the opening marker, capturing `root` and the `pr=` list. */ -const OPEN_MARKER_PATTERN = //; +/** + * Matches just the opening marker, capturing `root` and the `pr=` list. Each + * `pr=` entry is either `member` or `member:parent`, so the character class + * admits `:` — old flat markers (`pr=82,83,84`) still match and parse as + * members with no recorded parent (structure then comes from the live graph). + */ +const OPEN_MARKER_PATTERN = //; /** - * Parse the opening marker's `root` and ordered `pr=` membership list — but only - * inside a COMPLETE region (both markers present), so a stray or pasted opening - * marker without its closing tag is not mistaken for a managed region. + * Parse the opening marker's `root`, ordered `pr=` membership list, and any + * recorded `member:parent` topology — but only inside a COMPLETE region (both + * markers present), so a stray or pasted opening marker without its closing tag + * is not mistaken for a managed region. + * + * Returns `{ root, members, parents }` where `parents` is a Map of member → + * recorded parent for entries that carried a `:parent` suffix. A legacy flat + * marker yields an empty `parents` map — fully backward compatible. */ function parseStackComment(body) { const region = getRegion(body); @@ -49,11 +63,47 @@ function parseStackComment(body) { return undefined; } const root = Number(match[1]); - const members = match[2] - .split(",") - .filter((part) => part.length > 0) - .map(Number); - return { root, members }; + const members = []; + const parents = new Map(); + for (const part of match[2].split(",")) { + if (part.length === 0) { + continue; + } + const [memberText, parentText] = part.split(":"); + const member = Number(memberText); + if (!Number.isInteger(member)) { + continue; + } + members.push(member); + if (parentText !== undefined) { + const parent = Number(parentText); + if (Number.isInteger(parent)) { + parents.set(member, parent); + } + } + } + return { root, members, parents }; +} + +/** + * Consolidate the recorded `member → parent` topology across every PR body in + * the list. All PRs in a stack carry the same marker, so the union is + * consistent; on the rare disagreement, last-write wins. Legacy flat markers + * contribute nothing, so a stack only gains durable topology once it has been + * reconciled at least once under this format. + */ +function gatherRecordedParents(pullRequests) { + const parents = new Map(); + for (const pullRequest of pullRequests) { + const marker = parseStackComment(pullRequest.body); + if (!marker) { + continue; + } + for (const [member, parent] of marker.parents) { + parents.set(member, parent); + } + } + return parents; } /** Return the existing region (including both markers), or `undefined` if absent. */ @@ -107,24 +157,58 @@ function spliceRegion(body, region) { const HERE_PREFIX = "➡️ "; // ➡️ const HERE_SUFFIX = " (you are here)"; +const MERGED_SUFFIX = " ✅ merged"; +const CLOSED_SUFFIX = " ⛔ closed"; const STACK_HEADING = "**Stack** (root → tip):"; // root → tip -/** Render the full `` … `` region, or `''` for a non-stacked PR. */ -function renderRegion(tree, currentNumber) { +/** + * Render the full `` … `` region, or `''` for a + * non-stacked PR. + * + * `stateOf(number)` returns `'merged' | 'closed' | 'open'` (default `'open'`), + * so merged/closed ancestors stay in the tree with a status marker instead of + * being pruned. The `pr=` list encodes topology as `member:parent` (the root + * has no `:parent`), so the structure is recorded and survives retargeting. + */ +function renderRegion(tree, currentNumber, stateOf = () => "open") { if (tree.members.length <= 1) { return ""; } - const open = ``; + const encodeMember = (number_) => { + const parent = tree.parentOf.get(number_); + return parent === undefined ? String(number_) : `${number_}:${parent}`; + }; + const open = ``; const lines = [open, STACK_HEADING, ""]; + const stateSuffix = (number_) => { + switch (stateOf(number_)) { + case "merged": + return MERGED_SUFFIX; + case "closed": + return CLOSED_SUFFIX; + default: + return ""; + } + }; + + // `buildTree` yields an acyclic tree, but guard the recursion defensively so a + // hand-built or malformed `childrenOf` can never spin the workflow forever. + const rendered = new Set(); const renderNode = (number_, depth) => { + if (rendered.has(number_)) { + return; + } + rendered.add(number_); const indent = " ".repeat(depth); - lines.push( + const label = number_ === currentNumber - ? `${indent}- ${HERE_PREFIX}#${number_}${HERE_SUFFIX}` - : `${indent}- #${number_}` - ); + ? `${HERE_PREFIX}#${number_}${HERE_SUFFIX}` + : `#${number_}${stateSuffix(number_)}`; + lines.push(`${indent}- ${label}`); for (const child of tree.childrenOf.get(number_) ?? []) { renderNode(child, depth + 1); } @@ -136,7 +220,29 @@ function renderRegion(tree, currentNumber) { } // --------------------------------------------------------------------------- -// Tree derivation (pure, from the open-PR list) +// Tree derivation — additive topology (live edges + recorded parents) +// +// The tree is ADDITIVE: once a PR joins a stack it stays in the breadcrumb, even +// after it merges. Two facts feed the shape, and the first that answers wins: +// +// 1. RECORDED parent — the marker encodes topology as `member:parent` +// (`pr=82,83:82,84:83`). This is durable: it does not change when a branch +// is deleted or a PR is retargeted. +// 2. LIVE parent — the PR whose head branch is this PR's base. This reflects +// the current GitHub graph and seeds the topology for a brand-new member +// that no marker has recorded yet. +// +// Why recorded must win: when a parent merges, GitHub deletes its branch and +// retargets the child onto the default branch — which SEVERS the live edge +// (child.base becomes `main`, so `findRoot` would call the child its own root +// and the merged parent would vanish). The recorded parent survives that, so +// `buildTree` climbs it back to the true root and keeps the merged ancestor in +// the tree (rendered `✅ merged`). Because the marker is written while the edges +// are still live, the topology is captured BEFORE any retarget can sever it. +// +// A legacy flat marker (`pr=82,83,84`) records no parents, so such a stack +// derives purely from the live graph — identical to the pre-topology behavior — +// and self-migrates to the recorded form on its next reconcile. // --------------------------------------------------------------------------- function indexPullRequests(pullRequests) { @@ -173,40 +279,125 @@ function findRoot(startNumber, pullRequests, defaultBranch) { return current.number; } -/** Build the tree rooted at `rootNumber`: DFS pre-order members + sorted child map. */ +/** + * Build the provenance tree for `rootNumber`'s stack. + * + * Each member's parent is its RECORDED parent (from the marker topology) when + * one exists, else its LIVE parent (the PR whose head branch is this PR's base). + * Recorded parents win so the structure survives a merged parent whose child + * GitHub retargeted onto the default branch — which severs the live base/head + * edge but not the recorded one. From `rootNumber` we climb to the true (top) + * root, then DFS down the combined child map, so merged/closed ancestors stay + * in the tree instead of being pruned. + * + * Legacy flat markers record no parents, so a not-yet-migrated stack derives + * purely from the live graph (unchanged behavior); the first reconcile writes + * the topology while the edges are still live, making it durable thereafter. + * + * Returns `{ root, members, childrenOf, parentOf }` — members in DFS pre-order, + * `parentOf` a Map of member → parent (the root is absent). + */ function buildTree(rootNumber, pullRequests) { - const { byNumber } = indexPullRequests(pullRequests); + const { byNumber, byHead } = indexPullRequests(pullRequests); + const recordedParents = gatherRecordedParents(pullRequests); - const childrenByHead = new Map(); - for (const pullRequest of pullRequests) { - const siblings = childrenByHead.get(pullRequest.baseRefName) ?? []; - siblings.push(pullRequest.number); - childrenByHead.set(pullRequest.baseRefName, siblings); + const liveParentOf = (number_) => { + const node = byNumber.get(number_); + const parent = node ? byHead.get(node.baseRefName) : undefined; + return parent ? parent.number : undefined; + }; + // The LIVE edge wins — it is authoritative and not user-editable. A RECORDED + // parent only fills a SEVERED edge (a PR with no live parent), and only when + // it points at a KNOWN merged/closed ancestor — the sole legitimate case (a + // parent that merged and had its child retargeted onto the default branch). + // PR bodies are user-editable, so this containment stops a crafted marker from + // re-parenting an unrelated PR: it can neither override a live edge nor attach + // a PR under an open one. + const parentOf = (number_) => { + const live = liveParentOf(number_); + if (live !== undefined) { + return live; + } + const recorded = recordedParents.get(number_); + if (recorded === undefined) { + return undefined; + } + const recordedParent = byNumber.get(recorded); + const parentIsMergedOrClosed = + recordedParent !== undefined && + recordedParent.state !== undefined && + recordedParent.state !== "open"; + return parentIsMergedOrClosed ? recorded : undefined; + }; + + // Climb to the true root (recorded topology may point above `rootNumber` + // once an ancestor has merged and its child was retargeted). + let trueRoot = rootNumber; + const climbed = new Set(); + for ( + let parent = parentOf(trueRoot); + parent !== undefined && !climbed.has(trueRoot); + parent = parentOf(trueRoot) + ) { + climbed.add(trueRoot); + trueRoot = parent; + } + + // Every number we know of — live PRs plus any named only by recorded topology + // (e.g. a merged root that has dropped out of the open-PR list). + const known = new Set(byNumber.keys()); + for (const [member, parent] of recordedParents) { + known.add(member); + known.add(parent); } + // Invert parent → child, then DFS pre-order from the true root. const childrenOf = new Map(); + for (const number_ of known) { + const parent = parentOf(number_); + if (parent !== undefined && parent !== number_) { + childrenOf.set(parent, [...(childrenOf.get(parent) ?? []), number_]); + } + } + for (const [parent, children] of childrenOf) { + childrenOf.set( + parent, + children.toSorted((a, b) => a - b) + ); + } + const members = []; + const parentMap = new Map(); const visited = new Set(); - - const visit = (number_) => { - const node = byNumber.get(number_); - // Guard against a base/head cycle (A←B and B←A): never visit a PR twice. - if (!node || visited.has(number_)) { + const visit = (number_, parent) => { + // Guard against a topology cycle: never visit a number twice. + if (visited.has(number_)) { return; } visited.add(number_); members.push(number_); - const childNumbers = (childrenByHead.get(node.headRefName) ?? []) - .filter((candidate) => candidate !== number_) - .toSorted((a, b) => a - b); - childrenOf.set(number_, childNumbers); - for (const child of childNumbers) { - visit(child); + if (parent !== undefined) { + parentMap.set(number_, parent); + } + for (const child of childrenOf.get(number_) ?? []) { + visit(child, number_); } }; + visit(trueRoot, undefined); + + // Scope the child map to this tree's members, guaranteeing an entry (possibly + // empty) for every member — including leaves. + const scopedChildren = new Map(); + for (const number_ of members) { + scopedChildren.set(number_, childrenOf.get(number_) ?? []); + } - visit(rootNumber); - return { root: rootNumber, members, childrenOf }; + return { + root: trueRoot, + members, + childrenOf: scopedChildren, + parentOf: parentMap, + }; } /** Find the root of `triggerNumber`'s stack, then build the tree. */ @@ -296,6 +487,20 @@ async function reconcile(root, deps) { ); const tree = buildTree(root, pullRequests); + // Per-member status for rendering: a member with no PR object (named only by + // recorded topology) or an open one is unannotated; a closed one is shown as + // merged vs. plain-closed via its `merged` flag. + const stateOf = (number_) => { + const pullRequest = byNumber.get(number_); + if (!pullRequest || pullRequest.state === undefined) { + return "open"; + } + if (pullRequest.state === "open") { + return "open"; + } + return pullRequest.merged ? "merged" : "closed"; + }; + const updated = []; const skipped = []; const frozen = []; @@ -314,7 +519,7 @@ async function reconcile(root, deps) { frozen.push(number_); continue; } - const region = renderRegion(tree, number_); + const region = renderRegion(tree, number_, stateOf); const plan = planUpdate(pullRequest, region); if (!plan.changed) { skipped.push(number_); @@ -355,6 +560,7 @@ function hasFailedStackJob(jobs) { module.exports = { parseStackComment, + gatherRecordedParents, getRegion, spliceRegion, renderRegion, @@ -368,5 +574,7 @@ module.exports = { hasFailedStackJob, HERE_PREFIX, HERE_SUFFIX, + MERGED_SUFFIX, + CLOSED_SUFFIX, STACK_HEADING, }; diff --git a/.github/scripts/stack-breadcrumb.test.cjs b/.github/scripts/stack-breadcrumb.test.cjs index dc55693f..adede5d0 100644 --- a/.github/scripts/stack-breadcrumb.test.cjs +++ b/.github/scripts/stack-breadcrumb.test.cjs @@ -1,3 +1,7 @@ +// SPDX-License-Identifier: MIT +// Adapted from the taskless/taskless stack-breadcrumb implementation +// (@taskless/stack-breadcrumb) and brought into this repository under its MIT +// license, with permission. "use strict"; const test = require("node:test"); @@ -5,6 +9,7 @@ const assert = require("node:assert/strict"); const { parseStackComment, + gatherRecordedParents, getRegion, spliceRegion, renderRegion, @@ -21,11 +26,17 @@ const { const DEFAULT_BRANCH = "main"; function pr(number, head, base, body = "") { - return { number, title: `PR ${number}`, headRefName: head, baseRefName: base, body }; + return { + number, + title: `PR ${number}`, + headRefName: head, + baseRefName: base, + body, + }; } const REGION = [ - "", + "", "**Stack** (root → tip):", "", "- #10", @@ -34,17 +45,44 @@ const REGION = [ "", ].join("\n"); -test("parseStackComment: parses root and ordered membership", () => { +test("parseStackComment: parses root, membership, and recorded topology", () => { assert.deepEqual(parseStackComment(`intro\n\n${REGION}`), { root: 10, members: [10, 11, 12], + parents: new Map([ + [11, 10], + [12, 11], + ]), }); }); +test("parseStackComment: a legacy flat marker yields no recorded parents", () => { + assert.deepEqual( + parseStackComment("\nx\n"), + { root: 10, members: [10, 11, 12], parents: new Map() } + ); +}); + test("parseStackComment: parses an empty membership list", () => { assert.deepEqual( parseStackComment("\nx\n"), - { root: 10, members: [] } + { root: 10, members: [], parents: new Map() } + ); +}); + +test("gatherRecordedParents: consolidates topology across bodies", () => { + const region = "\nx\n"; + const prs = [ + pr(11, "b", "main", region), + pr(12, "c", "b", region), + pr(20, "x", "main", "no marker here"), + ]; + assert.deepEqual( + [...gatherRecordedParents(prs)], + [ + [11, 10], + [12, 11], + ] ); }); @@ -69,7 +107,10 @@ test("getRegion: undefined when absent", () => { }); test("spliceRegion: appends to a body with none", () => { - assert.equal(spliceRegion("Original body.", REGION), `Original body.\n\n${REGION}`); + assert.equal( + spliceRegion("Original body.", REGION), + `Original body.\n\n${REGION}` + ); }); test("spliceRegion: returns just the region for an empty body", () => { @@ -140,7 +181,10 @@ test("findRoot: resolves from any branch of a branching tree", () => { }); test("findRoot: a PR whose base has no open PR is its own root", () => { - assert.equal(findRoot(11, [pr(11, "b", "deleted-parent")], DEFAULT_BRANCH), 11); + assert.equal( + findRoot(11, [pr(11, "b", "deleted-parent")], DEFAULT_BRANCH), + 11 + ); }); test("findRoot: undefined for a PR that is not open", () => { @@ -188,7 +232,11 @@ test("findAllRoots: empty when there are no open PRs", () => { test("resolveAffectedRoots: open trigger returns its own root", () => { assert.deepEqual( - resolveAffectedRoots({ number: 12, baseRefName: "b" }, chain, DEFAULT_BRANCH), + resolveAffectedRoots( + { number: 12, baseRefName: "b" }, + chain, + DEFAULT_BRANCH + ), [10] ); }); @@ -196,7 +244,11 @@ test("resolveAffectedRoots: open trigger returns its own root", () => { test("resolveAffectedRoots: parent route when a leaf closes", () => { const open = [pr(10, "a", DEFAULT_BRANCH), pr(11, "b", "a")]; assert.deepEqual( - resolveAffectedRoots({ number: 12, baseRefName: "b" }, open, DEFAULT_BRANCH), + resolveAffectedRoots( + { number: 12, baseRefName: "b" }, + open, + DEFAULT_BRANCH + ), [10] ); }); @@ -299,11 +351,12 @@ test("reconcile: freezes a merged member but keeps it in the open PR's breadcrum }); test("reconcile: a lone open root with only merged descendants still renders", async () => { - // The whole stack has merged except the root — the breadcrumb must NOT clear. + // The whole stack has merged except the root — the breadcrumb must NOT clear, + // and the merged descendants stay listed with a merged marker. const prs = [ { ...pr(10, "a", "main"), state: "open" }, - { ...pr(11, "b", "a"), state: "closed" }, - { ...pr(12, "c", "b"), state: "closed" }, + { ...pr(11, "b", "a"), state: "closed", merged: true }, + { ...pr(12, "c", "b"), state: "closed", merged: true }, ]; const calls = []; const result = await reconcile(10, { @@ -312,7 +365,101 @@ test("reconcile: a lone open root with only merged descendants still renders", a }); assert.deepEqual(result.updated, [10]); assert.deepEqual(result.frozen, [11, 12]); - assert.match(calls[0][1], /pr=10,11,12/); + assert.match(calls[0][1], /pr=10,11:10,12:11/); + assert.match(calls[0][1], /#11 ✅ merged/); + assert.match(calls[0][1], /#12 ✅ merged/); +}); + +test("renderRegion: annotates merged/closed members and encodes topology", () => { + const stateOf = (number_) => + number_ === 11 ? "merged" : number_ === 12 ? "closed" : "open"; + const region = renderRegion(buildTree(10, chain), 10, stateOf); + assert.match(region, /pr=10,11:10,12:11/); + assert.match(region, /#11 ✅ merged/); + assert.match(region, /#12 ⛔ closed/); +}); + +test("buildTree: keeps a merged root after its child was retargeted", () => { + // #10 merged (root, branch a); #11 open but GitHub retargeted its base to + // main when #10 merged — the live base/head edge #10→#11 is gone. The + // recorded topology (member:parent) restores it, so #10 stays the root. + const marker = "\nx\n"; + const prs = [ + { ...pr(10, "a", DEFAULT_BRANCH, marker), state: "closed", merged: true }, + { ...pr(11, "b", DEFAULT_BRANCH, marker), state: "open" }, // retargeted to main + { ...pr(12, "c", "b", marker), state: "open" }, + ]; + // The dispatched root is the live root #11; buildTree climbs to #10. + const tree = buildTree(11, prs); + assert.equal(tree.root, 10); + assert.deepEqual(tree.members, [10, 11, 12]); + assert.equal(tree.parentOf.get(11), 10); + assert.equal(tree.parentOf.get(12), 11); +}); + +test("buildTree: a crafted marker cannot re-parent a PR under an open one", () => { + // A hand-edited body claims open #50 is a child of open #99. #50 has no live + // parent (base=main), but the recorded parent is OPEN, so it is NOT honored — + // #50 stays its own root instead of being hijacked into #99's tree. + const crafted = "\nx\n"; + const prs = [ + { ...pr(99, "x", DEFAULT_BRANCH, crafted), state: "open" }, + { ...pr(50, "y", DEFAULT_BRANCH, crafted), state: "open" }, + ]; + const tree = buildTree(50, prs); + assert.equal(tree.root, 50); + assert.deepEqual(tree.members, [50]); +}); + +test("buildTree: a recorded parent never overrides a live edge", () => { + // #11 has a live parent (#10 via base "a"); a body claiming a different + // recorded parent must not win over the authoritative live edge. + const lie = "\nx\n"; + const prs = [ + { ...pr(10, "a", DEFAULT_BRANCH), state: "open" }, + { ...pr(11, "b", "a", lie), state: "open" }, + ]; + const tree = buildTree(10, prs); + assert.equal(tree.root, 10); + assert.equal(tree.parentOf.get(11), 10); +}); + +test("renderRegion: terminates on a malformed cyclic childrenOf", () => { + // Defensive: a hand-built cyclic tree must not spin forever. + const cyclic = { + root: 1, + members: [1, 2], + parentOf: new Map([[2, 1]]), + childrenOf: new Map([ + [1, [2]], + [2, [1]], // cycle back to the root + ]), + }; + const region = renderRegion(cyclic, 1); + assert.match(region, /#1/); + assert.match(region, /#2/); +}); + +test("reconcile: keeps a merged, retargeted-away root in the open breadcrumbs", async () => { + const marker = "\nx\n"; + const prs = [ + { ...pr(10, "a", DEFAULT_BRANCH, marker), state: "closed", merged: true }, + { ...pr(11, "b", DEFAULT_BRANCH, marker), state: "open" }, + { ...pr(12, "c", "b", marker), state: "open" }, + ]; + const calls = []; + const result = await reconcile(11, { + pullRequests: prs, + writeBody: (number_, body) => calls.push([number_, body]), + }); + assert.deepEqual(result.frozen, [10]); // merged root never rewritten + assert.deepEqual( + result.updated.toSorted((a, b) => a - b), + [11, 12] + ); + const body11 = calls.find(([number_]) => number_ === 11)[1]; + assert.match(body11, /#10 ✅ merged/); // provenance kept + assert.match(body11, /pr=10,11:10,12:11/); // topology recorded forward }); test("hasFailedStackJob: true when a stack-prefixed job failed", () => { diff --git a/.github/workflows/stack-breadcrumb.yml b/.github/workflows/stack-breadcrumb.yml index f31d1de3..dde58668 100644 --- a/.github/workflows/stack-breadcrumb.yml +++ b/.github/workflows/stack-breadcrumb.yml @@ -1,3 +1,6 @@ +# SPDX-License-Identifier: MIT +# Adapted from the taskless/taskless stack-breadcrumb workflow and brought into +# this repository under its MIT license, with permission. name: Stack Breadcrumb # Self-contained stacked-PR breadcrumb. All logic lives in a zero-dependency @@ -194,6 +197,10 @@ jobs: baseRefName: pr.base.ref, body: pr.body ?? "", state: pr.state, // "closed" (merged or closed) + // Distinguish merged vs. plain-closed. `merged_at` is + // always present on a merged PR; the boolean `merged` is + // not reliably exposed on every response shape. + merged: pr.merged_at != null, }; } catch (error) { core.info(`stack-breadcrumb: could not fetch historical PR #${number}: ${String(error)}`); From fe6d374af4de849ffc992cf6d5e168572a444073 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Thu, 9 Jul 2026 18:52:00 -0700 Subject: [PATCH 2/3] ci(stack-breadcrumb): refresh from taskless/taskless main Pull the newer stack-breadcrumb improvements the team landed on taskless/taskless main since the initial port, reformatted to our Prettier style with the MIT/provenance headers preserved. New over the previous version: - Breadcrumb titles: entries render bare `#N` so GitHub autolinks the title + state, instead of a plain number. - Carry-forward job: on a stack member's merge, embed its body into its parent PR (keyed), so an incrementally-landed stack keeps its history. The job runs with contents:read (top-level `permissions: {}` grants no default) and skips merges into the default branch. - Topology hardening against crafted markers and cycles. Repo-agnostic (owner/repo/default-branch all resolved dynamically). All 59 unit tests pass via `node --test`. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/stack-breadcrumb.cjs | 137 ++++++++++++++++------ .github/scripts/stack-breadcrumb.test.cjs | 111 ++++++++++++++++-- .github/workflows/stack-breadcrumb.yml | 89 +++++++++++++- 3 files changed, 287 insertions(+), 50 deletions(-) diff --git a/.github/scripts/stack-breadcrumb.cjs b/.github/scripts/stack-breadcrumb.cjs index e2dc9414..aa230321 100644 --- a/.github/scripts/stack-breadcrumb.cjs +++ b/.github/scripts/stack-breadcrumb.cjs @@ -157,20 +157,20 @@ function spliceRegion(body, region) { const HERE_PREFIX = "➡️ "; // ➡️ const HERE_SUFFIX = " (you are here)"; -const MERGED_SUFFIX = " ✅ merged"; -const CLOSED_SUFFIX = " ⛔ closed"; const STACK_HEADING = "**Stack** (root → tip):"; // root → tip /** * Render the full `` … `` region, or `''` for a * non-stacked PR. * - * `stateOf(number)` returns `'merged' | 'closed' | 'open'` (default `'open'`), - * so merged/closed ancestors stay in the tree with a status marker instead of - * being pruned. The `pr=` list encodes topology as `member:parent` (the root - * has no `:parent`), so the structure is recorded and survives retargeting. + * Each node is a BARE `#N`: GitHub's own autolink expands a `#N` reference to + * the PR's title AND a state icon (open / merged / closed), so adding our own + * title text or `✅ merged` suffix only DUPLICATES what GitHub already renders. + * We add only the `➡️ … (you are here)` marker for the current PR, which GitHub + * cannot know. The `pr=` list still encodes topology as `member:parent` (the + * root has no `:parent`) so the structure is recorded and survives retargeting. */ -function renderRegion(tree, currentNumber, stateOf = () => "open") { +function renderRegion(tree, currentNumber) { if (tree.members.length <= 1) { return ""; } @@ -184,17 +184,6 @@ function renderRegion(tree, currentNumber, stateOf = () => "open") { .join(",")} -->`; const lines = [open, STACK_HEADING, ""]; - const stateSuffix = (number_) => { - switch (stateOf(number_)) { - case "merged": - return MERGED_SUFFIX; - case "closed": - return CLOSED_SUFFIX; - default: - return ""; - } - }; - // `buildTree` yields an acyclic tree, but guard the recursion defensively so a // hand-built or malformed `childrenOf` can never spin the workflow forever. const rendered = new Set(); @@ -207,7 +196,7 @@ function renderRegion(tree, currentNumber, stateOf = () => "open") { const label = number_ === currentNumber ? `${HERE_PREFIX}#${number_}${HERE_SUFFIX}` - : `#${number_}${stateSuffix(number_)}`; + : `#${number_}`; lines.push(`${indent}- ${label}`); for (const child of tree.childrenOf.get(number_) ?? []) { renderNode(child, depth + 1); @@ -219,6 +208,94 @@ function renderRegion(tree, currentNumber, stateOf = () => "open") { return lines.join("\n"); } +// --------------------------------------------------------------------------- +// PR carry-forward — a merged child's body embedded in its parent +// +// When a stack member merges INTO ITS PARENT (an atomic collapse, tip→root), +// its body is carried into the parent's PR body as a keyed region below the +// stack tree, so the PR that finally lands on the default branch holds the full +// legacy of everything it brings in. Regions are FLAT and keyed by PR number: +// carrying a child also re-homes the child's own carried regions as top-level +// siblings, and every region is upserted (replaced by its key), so the set is +// deduped no matter how many times this runs. Incremental delivery (each PR +// merges straight to the default branch) has no parent PR to carry into, so +// this is a no-op there — correct, since each slice stands alone. +// --------------------------------------------------------------------------- + +/** One carried region for a specific PR number (a known key → simple regex). */ +function prRegionPattern(number_) { + return new RegExp(`[\\S\\s]*?`); +} + +/** Any carried region; the open/close numbers must agree (backreference). */ +const ANY_PR_REGION_PATTERN = /[\S\s]*?/g; + +/** Every carried region already in `body`, in order: `{ number, region }[]`. */ +function extractCarriedRegions(body) { + const regions = []; + for (const match of body.matchAll(ANY_PR_REGION_PATTERN)) { + regions.push({ number: Number(match[1]), region: match[0] }); + } + return regions; +} + +/** + * A body's own description — the human-written part, with the managed regions + * (the stack tree and any carried `` regions) removed and spacing + * tidied. + */ +function ownDescription(body) { + return body + .replace(REGION_PATTERN, "") + .replace(ANY_PR_REGION_PATTERN, "") + .replace(/\n{3,}/g, "\n\n") + .trim(); +} + +/** Wrap a description as a carried region for `number_`. */ +function renderCarriedRegion(number_, description) { + return [ + ``, + `# Contains #${number_}`, + "", + description, + ``, + ].join("\n"); +} + +/** + * Upsert a carried region into `body` (keyed by PR number): replace an existing + * region for that number in place, else append it below the stack tree (at the + * end — the tree, written first, stays above). Idempotent. + */ +function upsertCarriedRegion(body, number_, region) { + const pattern = prRegionPattern(number_); + if (pattern.test(body)) { + // Function replacer so `$` sequences in the carried body are literal. + return body.replace(pattern, () => region); + } + const trimmed = body.replace(/\s+$/, ""); + return trimmed.length === 0 ? region : `${trimmed}\n\n${region}`; +} + +/** + * Carry a merged child's body into `parentBody`: add the child's own + * description as ``, and re-home the child's already-carried + * regions as flat top-level siblings — each upserted by key, so the parent ends + * with a deduped, flat set of every PR in the merged subtree, below the tree. + */ +function carryForward(parentBody, childNumber, childBody) { + let body = upsertCarriedRegion( + parentBody, + childNumber, + renderCarriedRegion(childNumber, ownDescription(childBody)) + ); + for (const { number, region } of extractCarriedRegions(childBody)) { + body = upsertCarriedRegion(body, number, region); + } + return body; +} + // --------------------------------------------------------------------------- // Tree derivation — additive topology (live edges + recorded parents) // @@ -487,20 +564,6 @@ async function reconcile(root, deps) { ); const tree = buildTree(root, pullRequests); - // Per-member status for rendering: a member with no PR object (named only by - // recorded topology) or an open one is unannotated; a closed one is shown as - // merged vs. plain-closed via its `merged` flag. - const stateOf = (number_) => { - const pullRequest = byNumber.get(number_); - if (!pullRequest || pullRequest.state === undefined) { - return "open"; - } - if (pullRequest.state === "open") { - return "open"; - } - return pullRequest.merged ? "merged" : "closed"; - }; - const updated = []; const skipped = []; const frozen = []; @@ -519,7 +582,7 @@ async function reconcile(root, deps) { frozen.push(number_); continue; } - const region = renderRegion(tree, number_, stateOf); + const region = renderRegion(tree, number_); const plan = planUpdate(pullRequest, region); if (!plan.changed) { skipped.push(number_); @@ -572,9 +635,11 @@ module.exports = { planUpdate, reconcile, hasFailedStackJob, + extractCarriedRegions, + ownDescription, + upsertCarriedRegion, + carryForward, HERE_PREFIX, HERE_SUFFIX, - MERGED_SUFFIX, - CLOSED_SUFFIX, STACK_HEADING, }; diff --git a/.github/scripts/stack-breadcrumb.test.cjs b/.github/scripts/stack-breadcrumb.test.cjs index adede5d0..eae42e38 100644 --- a/.github/scripts/stack-breadcrumb.test.cjs +++ b/.github/scripts/stack-breadcrumb.test.cjs @@ -21,6 +21,10 @@ const { planUpdate, reconcile, hasFailedStackJob, + extractCarriedRegions, + ownDescription, + upsertCarriedRegion, + carryForward, } = require("./stack-breadcrumb.cjs"); const DEFAULT_BRANCH = "main"; @@ -310,6 +314,7 @@ test("reconcile: writes only changed bodies and reports updated/skipped", async test("reconcile: skips a PR whose body already matches", async () => { const twoChain = [pr(10, "a", "main"), pr(11, "b", "a")]; const tree = buildTree(10, twoChain); + // Seed with the same render reconcile produces, so the body matches → skipped. const seeded = twoChain.map((p) => p.number === 10 ? { ...p, body: renderRegion(tree, 10) } : p ); @@ -366,17 +371,20 @@ test("reconcile: a lone open root with only merged descendants still renders", a assert.deepEqual(result.updated, [10]); assert.deepEqual(result.frozen, [11, 12]); assert.match(calls[0][1], /pr=10,11:10,12:11/); - assert.match(calls[0][1], /#11 ✅ merged/); - assert.match(calls[0][1], /#12 ✅ merged/); + assert.match(calls[0][1], /- #11\b/); // merged member still listed + assert.match(calls[0][1], /- #12\b/); }); -test("renderRegion: annotates merged/closed members and encodes topology", () => { - const stateOf = (number_) => - number_ === 11 ? "merged" : number_ === 12 ? "closed" : "open"; - const region = renderRegion(buildTree(10, chain), 10, stateOf); +test("renderRegion: renders bare #N refs and encodes topology (no title/status)", () => { + // GitHub autolinks a #N reference to the PR title + a state icon, so the tree + // stays bare — no appended title, no ✅/⛔ suffix — while the marker still + // records `member:parent` topology. + const region = renderRegion(buildTree(10, chain), 10); assert.match(region, /pr=10,11:10,12:11/); - assert.match(region, /#11 ✅ merged/); - assert.match(region, /#12 ⛔ closed/); + assert.match(region, /- ➡️ #10 \(you are here\)/); + assert.match(region, /- #11\n/); + assert.match(region, /- #12/); + assert.doesNotMatch(region, /✅|⛔|merged|closed/); }); test("buildTree: keeps a merged root after its child was retargeted", () => { @@ -458,7 +466,7 @@ test("reconcile: keeps a merged, retargeted-away root in the open breadcrumbs", [11, 12] ); const body11 = calls.find(([number_]) => number_ === 11)[1]; - assert.match(body11, /#10 ✅ merged/); // provenance kept + assert.match(body11, /- #10\b/); // merged root still listed (provenance kept) assert.match(body11, /pr=10,11:10,12:11/); // topology recorded forward }); @@ -504,3 +512,88 @@ test("hasFailedStackJob: false for empty/missing jobs", () => { assert.equal(hasFailedStackJob([]), false); assert.equal(hasFailedStackJob(undefined), false); }); + +// ─── Titles in the tree ────────────────────────────────────────────────────── + +// ─── PR carry-forward ──────────────────────────────────────────────────────── + +const P85 = [ + "", + "# Contains #85", + "", + "Tip work.", + "", +].join("\n"); + +test("extractCarriedRegions: finds each carried region with its number", () => { + const body = `intro\n\n${P85}\n\n\n# Contains #84\nx\n`; + assert.deepEqual( + extractCarriedRegions(body).map((r) => r.number), + [85, 84] + ); +}); + +test("ownDescription: strips the stack tree and carried regions", () => { + const body = [ + "My description.", + "", + "\n(tree)\n", + "", + P85, + ].join("\n"); + assert.equal(ownDescription(body), "My description."); +}); + +test("upsertCarriedRegion: appends when absent, replaces in place when present", () => { + const appended = upsertCarriedRegion("Body.", 85, P85); + assert.equal(appended, `Body.\n\n${P85}`); + + const replacement = P85.replace("Tip work.", "Revised."); + const replaced = upsertCarriedRegion(appended, 85, replacement); + assert.match(replaced, /Revised\./); + assert.doesNotMatch(replaced, /Tip work\./); + // Still exactly one region for #85 (no duplication). + assert.equal(extractCarriedRegions(replaced).length, 1); +}); + +test("carryForward: flattens the merged subtree into the parent below the tree", () => { + // #84 already carries #85; merging #84 into #83 lands both, flat. + const parent83 = + "Parent 83 desc.\n\n\n(tree)\n"; + const child84 = [ + "Group 84 work.", + "", + "\n(tree)\n", + "", + P85, + ].join("\n"); + + const result = carryForward(parent83, 84, child84); + + const carried = extractCarriedRegions(result); + assert.deepEqual( + carried.map((r) => r.number), + [84, 85] + ); + // #84's region is its OWN description only (tree + #85 stripped out). + assert.match( + result, + /\n# Contains #84\n\nGroup 84 work\.\n/ + ); + // #85 re-homed verbatim as a flat sibling. + assert.ok(result.includes(P85)); + // Carried regions sit below the tree. + assert.ok( + result.indexOf("") < result.indexOf("") + ); +}); + +test("carryForward: is idempotent — re-carrying does not duplicate", () => { + const parent = + "Parent.\n\n\n(t)\n"; + const child = "Child work."; + const once = carryForward(parent, 2, child); + const twice = carryForward(once, 2, child); + assert.equal(once, twice); + assert.equal(extractCarriedRegions(twice).length, 1); +}); diff --git a/.github/workflows/stack-breadcrumb.yml b/.github/workflows/stack-breadcrumb.yml index dde58668..72d92b9c 100644 --- a/.github/workflows/stack-breadcrumb.yml +++ b/.github/workflows/stack-breadcrumb.yml @@ -16,6 +16,11 @@ name: Stack Breadcrumb # `stack`-prefixed check that a shape change left failing # on a PR that is no longer the tip. # +# A separate CARRY-FORWARD job runs on a member's MERGE: it embeds the merged +# PR's body into its parent PR as a keyed `` region below the tree, +# so an atomic collapse (tip→root) accumulates the full legacy on the PR that +# lands on the default branch. Incremental delivery has no parent → a no-op. +# # Stage 1 fires the dispatch with the built-in GITHUB_TOKEN: although that token # normally suppresses recursive workflow runs, `repository_dispatch` is a # documented exception, so stage 2 fires. NOTE: `repository_dispatch` always runs @@ -116,6 +121,84 @@ jobs: core.info(`dispatched reconcile for root #${root}`); } + carry-forward: + name: Carry a merged PR's body into its parent + # On a stack member's MERGE, embed its body into its parent PR as a keyed + # `` region below the tree, so an atomic collapse (tip→root) + # accumulates the full legacy on the PR that finally lands on the default + # branch. Incremental delivery (each PR merges straight to the default + # branch) has no open parent PR to carry into → a clean no-op. + if: >- + github.event_name == 'pull_request' + && github.event.action == 'closed' + && github.event.pull_request.merged == true + && github.event.pull_request.base.ref != github.event.repository.default_branch + && github.event.pull_request.head.repo.full_name == github.repository + && github.event.sender.login != 'github-actions[bot]' + runs-on: ubuntu-latest + permissions: + contents: read # actions/checkout needs this (top-level permissions: {} grants no defaults) + pull-requests: write # edit the parent PR body + concurrency: + # Serialize per PARENT branch (the body we write); convergent upserts make + # this safe, and we must not cancel a carry mid-write. + group: stack-carry-forward-${{ github.event.pull_request.base.ref }} + cancel-in-progress: false + steps: + # Trusted script from the default branch — never PR-supplied code. + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.repository.default_branch }} + + - uses: actions/github-script@v7 + with: + script: | + const scriptPath = `${process.env.GITHUB_WORKSPACE}/.github/scripts/stack-breadcrumb.cjs`; + let stack; + try { + stack = require(scriptPath); + } catch (error) { + if (error.code === "MODULE_NOT_FOUND") { + core.info("stack-breadcrumb script not on the default branch yet; skipping until merged."); + return; + } + throw error; + } + const { owner, repo } = context.repo; + const merged = context.payload.pull_request; + + // The parent is the open PR whose head branch is this PR's base. + const rawPulls = await github.paginate(github.rest.pulls.list, { + owner, repo, state: "open", per_page: 100, + }); + const parent = rawPulls.find( + (pr) => + pr.head.repo?.full_name === `${owner}/${repo}` && + pr.head.ref === merged.base.ref + ); + if (!parent) { + core.info(`#${merged.number} merged into "${merged.base.ref}", which has no open parent PR — nothing to carry (incremental land).`); + return; + } + + // Fetch fresh bodies — the event payload can lag a last-moment edit. + const [{ data: mergedFresh }, { data: parentFresh }] = await Promise.all([ + github.rest.pulls.get({ owner, repo, pull_number: merged.number }), + github.rest.pulls.get({ owner, repo, pull_number: parent.number }), + ]); + + const newBody = stack.carryForward( + parentFresh.body ?? "", + merged.number, + mergedFresh.body ?? "" + ); + if (newBody === (parentFresh.body ?? "")) { + core.info(`#${merged.number} already carried into #${parent.number}; nothing to do.`); + return; + } + await github.rest.pulls.update({ owner, repo, pull_number: parent.number, body: newBody }); + core.info(`carried #${merged.number} into parent #${parent.number}`); + reconcile: name: Reconcile breadcrumb across the stack if: github.event_name == 'repository_dispatch' || github.event_name == 'workflow_dispatch' @@ -196,11 +279,7 @@ jobs: headRefName: pr.head.ref, baseRefName: pr.base.ref, body: pr.body ?? "", - state: pr.state, // "closed" (merged or closed) - // Distinguish merged vs. plain-closed. `merged_at` is - // always present on a merged PR; the boolean `merged` is - // not reliably exposed on every response shape. - merged: pr.merged_at != null, + state: pr.state, // "closed" (merged or closed) — freezes rewrites }; } catch (error) { core.info(`stack-breadcrumb: could not fetch historical PR #${number}: ${String(error)}`); From 0d1ac4911393346fa61676beb0ef425781afe2ab Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Thu, 9 Jul 2026 22:32:08 -0700 Subject: [PATCH 3/3] docs(stack-breadcrumb): correct parent-precedence comments to match the code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tree-derivation block comment and buildTree docstring said the RECORDED parent wins, but the implementation (and the inline comment at the resolver) makes the LIVE edge authoritative — the recorded parent is a contained fallback used only for a severed edge that names a known merged/closed ancestor. That containment is a security property (PR bodies are user-editable, so a crafted marker must not re-parent an unrelated PR), so the stale "recorded must win" framing was not just imprecise — following it could reintroduce the crafted-marker hole the hardening pass closed. Also drop the "rendered ✅ merged" aside: entries render bare #N and GitHub supplies the status icon. Comment-only; no behavior change. Flagged by Copilot review on #56; applies cleanly back onto taskless/taskless (prose-only, quote-agnostic). Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/stack-breadcrumb.cjs | 42 +++++++++++++++------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/.github/scripts/stack-breadcrumb.cjs b/.github/scripts/stack-breadcrumb.cjs index aa230321..34be19c2 100644 --- a/.github/scripts/stack-breadcrumb.cjs +++ b/.github/scripts/stack-breadcrumb.cjs @@ -300,22 +300,25 @@ function carryForward(parentBody, childNumber, childBody) { // Tree derivation — additive topology (live edges + recorded parents) // // The tree is ADDITIVE: once a PR joins a stack it stays in the breadcrumb, even -// after it merges. Two facts feed the shape, and the first that answers wins: +// after it merges. Two facts feed the shape: // -// 1. RECORDED parent — the marker encodes topology as `member:parent` +// 1. LIVE parent — the PR whose head branch is this PR's base. This reflects +// the current GitHub graph and is authoritative: it is not user-editable. +// 2. RECORDED parent — the marker encodes topology as `member:parent` // (`pr=82,83:82,84:83`). This is durable: it does not change when a branch // is deleted or a PR is retargeted. -// 2. LIVE parent — the PR whose head branch is this PR's base. This reflects -// the current GitHub graph and seeds the topology for a brand-new member -// that no marker has recorded yet. // -// Why recorded must win: when a parent merges, GitHub deletes its branch and -// retargets the child onto the default branch — which SEVERS the live edge -// (child.base becomes `main`, so `findRoot` would call the child its own root -// and the merged parent would vanish). The recorded parent survives that, so -// `buildTree` climbs it back to the true root and keeps the merged ancestor in -// the tree (rendered `✅ merged`). Because the marker is written while the edges -// are still live, the topology is captured BEFORE any retarget can sever it. +// The LIVE parent wins. The RECORDED parent is only a fallback for a SEVERED +// edge: when a parent merges, GitHub deletes its branch and retargets the child +// onto the default branch (child.base becomes `main`), so `findRoot` would call +// the child its own root and the merged parent would vanish. In that one case — +// and only when the recorded parent names a KNOWN merged/closed ancestor — the +// recorded link is used, so `buildTree` climbs back to the true root and keeps +// the merged ancestor in the tree. Because PR bodies are user-editable, this +// containment (recorded never overrides a live edge, never attaches a PR under +// an open one) stops a crafted marker from re-parenting an unrelated PR. The +// marker is written while the edges are still live, so the topology is captured +// BEFORE any retarget can sever it. // // A legacy flat marker (`pr=82,83,84`) records no parents, so such a stack // derives purely from the live graph — identical to the pre-topology behavior — @@ -359,13 +362,14 @@ function findRoot(startNumber, pullRequests, defaultBranch) { /** * Build the provenance tree for `rootNumber`'s stack. * - * Each member's parent is its RECORDED parent (from the marker topology) when - * one exists, else its LIVE parent (the PR whose head branch is this PR's base). - * Recorded parents win so the structure survives a merged parent whose child - * GitHub retargeted onto the default branch — which severs the live base/head - * edge but not the recorded one. From `rootNumber` we climb to the true (top) - * root, then DFS down the combined child map, so merged/closed ancestors stay - * in the tree instead of being pruned. + * Each member's parent is its LIVE parent (the PR whose head branch is this + * PR's base) when one exists, else its RECORDED parent (from the marker + * topology) — and the recorded link is used only for a SEVERED edge that names + * a known merged/closed ancestor. The live edge is authoritative; the recorded + * parent is the durable fallback that keeps a merged parent (whose child GitHub + * retargeted onto the default branch) from vanishing. From `rootNumber` we climb + * to the true (top) root, then DFS down the combined child map, so merged/closed + * ancestors stay in the tree instead of being pruned. * * Legacy flat markers record no parents, so a not-yet-migrated stack derives * purely from the live graph (unchanged behavior); the first reconcile writes