Skip to content

fix(ci): Enforce canonical region order in stacked-PR bodies #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thecodedrift merged 2 commits into from
Jul 12, 2026
Merged

fix(ci): Enforce canonical region order in stacked-PR bodies #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bde42b
fix(ci): Enforce canonical region order in stacked-PR bodies
thecodedrift Jul 12, 2026
491a491
fix(ci): Preserve leading prose indentation in ownDescription
thecodedrift Jul 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 81 additions & 55 deletions .github/scripts/stack-breadcrumb.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,13 @@
*/

// ---------------------------------------------------------------------------
// Marker-region surgery
// Marker-region parsing
//
// Each managed PR body carries at most one region delimited by
// `<!-- stack root=N pr=… -->` … `<!-- /stack -->`. 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
// `<!-- stack root=N pr=… -->` … `<!-- /stack -->`. 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 `<!-- /stack -->`. */
Expand Down Expand Up @@ -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
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -241,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 `<!-- PR:N -->` regions) removed and spacing
* tidied.
* (the stack tree and any carried `<!-- PR:N -->` 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_`. */
Expand All @@ -265,8 +233,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_);
Expand All @@ -283,6 +253,12 @@ function upsertCarriedRegion(body, number_, region) {
* description as `<!-- PR:child -->`, 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(
Expand All @@ -293,7 +269,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 (<!-- stack … -->) — on top
// 2. the human description (everything unmanaged)
// 3. carried-forward legacy (<!-- PR:N --> 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);
Comment thread
thecodedrift marked this conversation as resolved.
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");
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -546,9 +567,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,
Expand Down Expand Up @@ -629,7 +655,7 @@ module.exports = {
parseStackComment,
gatherRecordedParents,
getRegion,
spliceRegion,
canonicalizeBody,
renderRegion,
findRoot,
buildTree,
Expand Down
115 changes: 87 additions & 28 deletions .github/scripts/stack-breadcrumb.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const {
parseStackComment,
gatherRecordedParents,
getRegion,
spliceRegion,
canonicalizeBody,
renderRegion,
findRoot,
buildTree,
Expand Down Expand Up @@ -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 = [
"<!-- PR:84 -->",
"# Contains #84",
"",
"Group 84 work.",
"<!-- /PR:84 -->",
].join("\n");
const CARRIED_85 = [
"<!-- PR:85 -->",
"# Contains #85",
"",
"Tip work.",
"<!-- /PR:85 -->",
].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 = "<!-- stack root=10 pr=10,11 -->\nold\n<!-- /stack -->";
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 =
"<!-- stack root=10 pr=10,11 -->\ncost is $1 and $& too\n<!-- /stack -->";
const body = "<!-- stack root=10 pr=10 -->\nold\n<!-- /stack -->";
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")];
Expand Down Expand Up @@ -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);
});

Expand Down Expand Up @@ -544,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 =
"<!-- stack root=1 pr=1,2:1 -->\n(tree)\n<!-- /stack -->\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}`);
Expand Down Expand Up @@ -597,3 +642,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 =
"<!-- stack root=82 pr=82,83:82,84:83 -->\n(tree)\n<!-- /stack -->";
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<!-- PR:84 -->\n# Contains #84\n\nGroup 84 work.\n<!-- /PR:84 -->`
);
});
Loading