From 4bde42bfa2ceb0f133e6a0229fe893beb0a9c6c4 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sun, 12 Jul 2026 13:07:59 -0700 Subject: [PATCH 1/2] fix(ci): Enforce canonical region order in stacked-PR bodies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked-PR bodies carry three managed regions — the stack breadcrumb, the human description, and carried-forward legacy — but their order was emergent: spliceRegion and upsertCarriedRegion replaced in place or appended, so a region stayed wherever it first landed. Add canonicalizeBody as the single layout authority: breadcrumb → description → carried-forward (ascending PR#). Route the sync path (planUpdate/reconcile) and the merge path (carryForward) through it so both converge to the same canonical body, and a git town sync reformats an existing PR — the one moment the order matters. A body with no managed regions is returned untouched, so plain non-stacked PRs are never rewritten. Retire spliceRegion, now subsumed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/stack-breadcrumb.cjs | 122 +++++++++++++--------- .github/scripts/stack-breadcrumb.test.cjs | 107 ++++++++++++++----- 2 files changed, 149 insertions(+), 80 deletions(-) diff --git a/.github/scripts/stack-breadcrumb.cjs b/.github/scripts/stack-breadcrumb.cjs index 34be19c..064e0dc 100644 --- a/.github/scripts/stack-breadcrumb.cjs +++ b/.github/scripts/stack-breadcrumb.cjs @@ -22,14 +22,13 @@ */ // --------------------------------------------------------------------------- -// Marker-region surgery +// Marker-region parsing // -// Each managed PR body carries at most one region delimited by -// `` … ``. Replacing a region in place -// leaves every other byte untouched, so a git-town breadcrumb or any other -// content is never disturbed. Appending trims the body's trailing whitespace -// before adding the region after a blank line; removing collapses the blank -// lines the region left behind and trims trailing whitespace. +// Each managed PR body carries at most one breadcrumb region delimited by +// `` … ``. The helpers here locate and +// parse that region. Placing it in a body — and ordering it against the other +// managed regions — is the job of `canonicalizeBody` (see "Canonical body +// layout" below), which is the single authority on where every region lands. // --------------------------------------------------------------------------- /** Matches the whole region, opening marker through ``. */ @@ -112,45 +111,6 @@ function getRegion(body) { return match ? match[0] : undefined; } -/** - * Splice `region` into `body`: - * - an empty `region` removes an existing region (non-stacked PR); - * - an existing region is replaced in place; - * - otherwise the region is appended after a blank line. - * Content outside the markers is preserved. - */ -function spliceRegion(body, region) { - const existing = getRegion(body); - - if (region.length === 0) { - if (!existing) { - return body; - } - // Remove the region and only the blank lines hugging it, rejoining the - // surrounding text with a single blank line. Spacing elsewhere in the body - // is left exactly as-is. - const start = body.indexOf(existing); - const before = body.slice(0, start).replace(/\n+$/, ""); - const after = body.slice(start + existing.length).replace(/^\n+/, ""); - if (before.length === 0) { - return after.trimEnd(); - } - if (after.length === 0) { - return before.trimEnd(); - } - return `${before}\n\n${after}`; - } - - if (existing) { - // Function replacer so `$` sequences in a PR title are not treated as - // `String.replace` special patterns ($&, $1, …). - return body.replace(REGION_PATTERN, () => region); - } - - const trimmed = body.trimEnd(); - return trimmed.length === 0 ? region : `${trimmed}\n\n${region}`; -} - // --------------------------------------------------------------------------- // Rendering // --------------------------------------------------------------------------- @@ -265,8 +225,10 @@ function renderCarriedRegion(number_, description) { /** * 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. + * region for that number in place, else append it. Final placement (below the + * breadcrumb and description, sorted by PR number) is settled by + * `canonicalizeBody`; this helper only guarantees the region is present exactly + * once. Idempotent. */ function upsertCarriedRegion(body, number_, region) { const pattern = prRegionPattern(number_); @@ -283,6 +245,12 @@ function upsertCarriedRegion(body, number_, region) { * 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. + * + * A final `canonicalizeBody` re-lays the accumulated body in the canonical order + * (breadcrumb → description → carried), keeping the parent's existing + * breadcrumb. This is convergent with the sync-time reconcile, which both run on + * a member's merge: whichever writes the parent body last, the result is the + * same canonical string. */ function carryForward(parentBody, childNumber, childBody) { let body = upsertCarriedRegion( @@ -293,7 +261,52 @@ function carryForward(parentBody, childNumber, childBody) { for (const { number, region } of extractCarriedRegions(childBody)) { body = upsertCarriedRegion(body, number, region); } - return body; + return canonicalizeBody(body, getRegion(body) ?? ""); +} + +// --------------------------------------------------------------------------- +// Canonical body layout +// +// A managed PR body always renders its regions in one fixed order, so a stack +// reads the same on every PR and reformatting is deterministic: +// +// 1. the stack breadcrumb () — on top +// 2. the human description (everything unmanaged) +// 3. carried-forward legacy ( blocks, ascending) — on the bottom +// +// `breadcrumb` is the breadcrumb region to place: `''` (or a non-string) omits +// it, so a PR that has left its stack drops the region. The description and any +// carried regions are taken from `body`; carried regions are sorted by PR number +// so the order is stable no matter which order they were carried in. The +// transform is idempotent — a body already in canonical form is returned +// unchanged. As a deliberate exception, a body with NOTHING managed (no +// breadcrumb to place, none present, no carried regions) is returned +// byte-for-byte, so a plain, non-stacked PR's description is never rewritten. +// --------------------------------------------------------------------------- + +function canonicalizeBody(body, breadcrumb) { + const hasBreadcrumb = typeof breadcrumb === "string" && breadcrumb.length > 0; + const carried = extractCarriedRegions(body).toSorted( + (a, b) => a.number - b.number + ); + // Nothing managed to place and nothing already present: leave the body exactly + // as-is. Only PRs that actually participate in the managed layout get + // reformatted, so a normal PR's prose (and its incidental whitespace) is safe. + if (!hasBreadcrumb && carried.length === 0 && getRegion(body) === undefined) { + return body; + } + const description = ownDescription(body); + const sections = []; + if (hasBreadcrumb) { + sections.push(breadcrumb); + } + if (description.length > 0) { + sections.push(description); + } + for (const { region } of carried) { + sections.push(region); + } + return sections.join("\n\n"); } // --------------------------------------------------------------------------- @@ -546,9 +559,14 @@ function findAllRoots(pullRequests, defaultBranch) { // Reconcile orchestration // --------------------------------------------------------------------------- -/** Compute the new body for one PR given its desired region (`''` removes it). */ +/** + * Compute the new body for one PR given its desired breadcrumb region (`''` + * removes it). The body is re-laid in canonical order (breadcrumb → description + * → carried), so a sync also normalizes the layout of an existing PR — the one + * moment the region order matters. + */ function planUpdate(pullRequest, region) { - const body = spliceRegion(pullRequest.body, region); + const body = canonicalizeBody(pullRequest.body, region); return { number: pullRequest.number, changed: body !== pullRequest.body, @@ -629,7 +647,7 @@ module.exports = { parseStackComment, gatherRecordedParents, getRegion, - spliceRegion, + canonicalizeBody, renderRegion, findRoot, buildTree, diff --git a/.github/scripts/stack-breadcrumb.test.cjs b/.github/scripts/stack-breadcrumb.test.cjs index eae42e3..815f173 100644 --- a/.github/scripts/stack-breadcrumb.test.cjs +++ b/.github/scripts/stack-breadcrumb.test.cjs @@ -11,7 +11,7 @@ const { parseStackComment, gatherRecordedParents, getRegion, - spliceRegion, + canonicalizeBody, renderRegion, findRoot, buildTree, @@ -110,51 +110,80 @@ test("getRegion: undefined when absent", () => { assert.equal(getRegion("no region here"), undefined); }); -test("spliceRegion: appends to a body with none", () => { +// A carried-forward legacy region, used by the canonical-layout tests below. +const CARRIED_84 = [ + "", + "# Contains #84", + "", + "Group 84 work.", + "", +].join("\n"); +const CARRIED_85 = [ + "", + "# Contains #85", + "", + "Tip work.", + "", +].join("\n"); + +test("canonicalizeBody: orders breadcrumb → description → carried", () => { assert.equal( - spliceRegion("Original body.", REGION), - `Original body.\n\n${REGION}` + canonicalizeBody("Original body.", REGION), + `${REGION}\n\nOriginal body.` ); }); -test("spliceRegion: returns just the region for an empty body", () => { - assert.equal(spliceRegion("", REGION), REGION); +test("canonicalizeBody: returns just the breadcrumb for an empty body", () => { + assert.equal(canonicalizeBody("", REGION), REGION); }); -test("spliceRegion: replaces an existing region in place", () => { - const oldRegion = "\nold\n"; - const body = `top\n\n${oldRegion}\n\nbottom`; - assert.equal(spliceRegion(body, REGION), `top\n\n${REGION}\n\nbottom`); +test("canonicalizeBody: reflows an out-of-order body into canonical order", () => { + // Worst case: carried region first, then description, then the breadcrumb. + const body = `${CARRIED_85}\n\nMy description.\n\n${REGION}`; + assert.equal( + canonicalizeBody(body, REGION), + `${REGION}\n\nMy description.\n\n${CARRIED_85}` + ); }); -test("spliceRegion: removes the region when given an empty region", () => { - assert.equal(spliceRegion(`keep this\n\n${REGION}`, ""), "keep this"); +test("canonicalizeBody: sorts carried regions ascending by PR number", () => { + // #85 appears before #84 in the input; canonical order sorts them 84, 85. + const body = `Desc.\n\n${CARRIED_85}\n\n${CARRIED_84}\n\n${REGION}`; + assert.equal( + canonicalizeBody(body, REGION), + `${REGION}\n\nDesc.\n\n${CARRIED_84}\n\n${CARRIED_85}` + ); }); -test("spliceRegion: no-op removing when no region exists", () => { - assert.equal(spliceRegion("nothing to remove", ""), "nothing to remove"); +test("canonicalizeBody: is idempotent on an already-canonical body", () => { + const canonical = `${REGION}\n\nDesc.\n\n${CARRIED_84}`; + assert.equal(canonicalizeBody(canonical, REGION), canonical); }); -test("spliceRegion: removing preserves unrelated blank runs elsewhere", () => { - // A 3+ newline run unrelated to the region must survive removal. - const body = `line1\n\n\n\nline2\n\n${REGION}`; - assert.equal(spliceRegion(body, ""), "line1\n\n\n\nline2"); +test("canonicalizeBody: empty breadcrumb drops the stack region", () => { + assert.equal(canonicalizeBody(`keep this\n\n${REGION}`, ""), "keep this"); }); -test("spliceRegion: removing a middle region rejoins with one blank line", () => { - assert.equal(spliceRegion(`top\n\n${REGION}\n\nbottom`, ""), "top\n\nbottom"); +test("canonicalizeBody: leaves a plain non-stacked body byte-for-byte", () => { + // No breadcrumb to place, none present, no carried regions → untouched, even + // with incidental trailing whitespace and blank runs a reformat would tidy. + const body = "just a normal PR\n\n\n\nwith odd spacing\n"; + assert.equal(canonicalizeBody(body, ""), body); }); -test("spliceRegion: skip-when-equal is a no-op", () => { - const body = `intro\n\n${REGION}\n\noutro`; - assert.equal(spliceRegion(body, REGION), body); +test("canonicalizeBody: keeps carried legacy even with no breadcrumb", () => { + // A collapsed root that has left its stack but still holds carried history. + assert.equal( + canonicalizeBody(`Desc.\n\n${CARRIED_84}`, ""), + `Desc.\n\n${CARRIED_84}` + ); }); -test("spliceRegion: does not treat $ sequences as replacement patterns", () => { +test("canonicalizeBody: does not treat $ sequences as replacement patterns", () => { const dollarRegion = "\ncost is $1 and $& too\n"; const body = "\nold\n"; - assert.equal(spliceRegion(body, dollarRegion), dollarRegion); + assert.equal(canonicalizeBody(body, dollarRegion), dollarRegion); }); const chain = [pr(10, "a", DEFAULT_BRANCH), pr(11, "b", "a"), pr(12, "c", "b")]; @@ -281,14 +310,22 @@ test("resolveAffectedRoots: nothing when a closed PR affects no open stack", () ); }); -test("planUpdate: appends the region and flags a change", () => { +test("planUpdate: places the breadcrumb above the description and flags a change", () => { const plan = planUpdate(pr(10, "a", "main", "intro"), REGION); assert.equal(plan.changed, true); - assert.ok(plan.body.includes(REGION)); + assert.equal(plan.body, `${REGION}\n\nintro`); }); -test("planUpdate: skips when the body already has the region", () => { +test("planUpdate: reflows a description-first body to canonical order", () => { + // An existing PR authored with the breadcrumb below the prose is reformatted + // on sync — the one moment the region order matters. const plan = planUpdate(pr(10, "a", "main", `intro\n\n${REGION}`), REGION); + assert.equal(plan.changed, true); + assert.equal(plan.body, `${REGION}\n\nintro`); +}); + +test("planUpdate: skips when the body is already canonical", () => { + const plan = planUpdate(pr(10, "a", "main", `${REGION}\n\nintro`), REGION); assert.equal(plan.changed, false); }); @@ -597,3 +634,17 @@ test("carryForward: is idempotent — re-carrying does not duplicate", () => { assert.equal(once, twice); assert.equal(extractCarriedRegions(twice).length, 1); }); + +test("carryForward: emits canonical order — breadcrumb, then description, then carried", () => { + // Parent authored description-first; carrying a child must hoist the + // breadcrumb above the prose, matching the sync-time layout so the two + // converge no matter which writes the parent body last. + const breadcrumb = + "\n(tree)\n"; + const parent83 = `Parent 83 desc.\n\n${breadcrumb}`; + const result = carryForward(parent83, 84, "Group 84 work."); + assert.equal( + result, + `${breadcrumb}\n\nParent 83 desc.\n\n\n# Contains #84\n\nGroup 84 work.\n` + ); +}); From 491a4915773f40e97046c432e2152ae24b22ca6e Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sun, 12 Jul 2026 16:51:27 -0700 Subject: [PATCH 2/2] fix(ci): Preserve leading prose indentation in ownDescription MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit canonicalizeBody derives the description via ownDescription, which used .trim() — stripping leading indentation from a managed PR's prose, so a description opening with an indented Markdown code block could be silently reflowed. Tidy only the whitespace the region surgery introduces: collapse seams, drop leading blank lines, and trim trailing whitespace, leaving the first line's indentation intact. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/stack-breadcrumb.cjs | 14 +++++++++++--- .github/scripts/stack-breadcrumb.test.cjs | 8 ++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/.github/scripts/stack-breadcrumb.cjs b/.github/scripts/stack-breadcrumb.cjs index 064e0dc..f033a98 100644 --- a/.github/scripts/stack-breadcrumb.cjs +++ b/.github/scripts/stack-breadcrumb.cjs @@ -201,15 +201,23 @@ function extractCarriedRegions(body) { /** * A body's own description — the human-written part, with the managed regions - * (the stack tree and any carried `` regions) removed and spacing - * tidied. + * (the stack tree and any carried `` regions) removed and the + * spacing their removal left behind tidied. + * + * Only whitespace introduced by the surgery is touched: 3+ newline runs (a seam + * where a region was cut from the middle) collapse to a blank line, leading + * blank lines (left when a region sat at the very top) are dropped, and trailing + * whitespace is trimmed. The prose is otherwise preserved verbatim — crucially, + * leading indentation on the first line survives, so a description that opens + * with an indented Markdown code block is not silently reflowed. */ function ownDescription(body) { return body .replace(REGION_PATTERN, "") .replace(ANY_PR_REGION_PATTERN, "") .replace(/\n{3,}/g, "\n\n") - .trim(); + .replace(/^\n+/, "") + .trimEnd(); } /** Wrap a description as a carried region for `number_`. */ diff --git a/.github/scripts/stack-breadcrumb.test.cjs b/.github/scripts/stack-breadcrumb.test.cjs index 815f173..bc84564 100644 --- a/.github/scripts/stack-breadcrumb.test.cjs +++ b/.github/scripts/stack-breadcrumb.test.cjs @@ -581,6 +581,14 @@ test("ownDescription: strips the stack tree and carried regions", () => { assert.equal(ownDescription(body), "My description."); }); +test("ownDescription: preserves leading indentation after a top region", () => { + // A breadcrumb at the very top must not cost the prose its leading indent — + // otherwise a description opening with an indented code block gets reflowed. + const body = + "\n(tree)\n\n\n const x = 1;\nplain line"; + assert.equal(ownDescription(body), " const x = 1;\nplain line"); +}); + test("upsertCarriedRegion: appends when absent, replaces in place when present", () => { const appended = upsertCarriedRegion("Body.", 85, P85); assert.equal(appended, `Body.\n\n${P85}`);