From c302428da273901297f76ac807b320565685d345 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sat, 4 Jul 2026 13:02:16 -0700 Subject: [PATCH 1/4] feat(ci): preserve merged PRs in the stack breadcrumb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The breadcrumb previously dropped merged PRs (tree built from open PRs only) and cleared entirely once a stack collapsed to its last open PR — losing the record of how the final merged-down PR was constructed. Now the reconcile step also pulls in the stack members recorded in existing breadcrumb markers that are no longer open, fetches them for tree structure, and keeps them in the rendered list. GitHub auto-renders the PR reference with its merged/closed badge, so no explicit status marker is needed. Merged/closed members are frozen — reconcile keeps them in the tree (so open PRs keep listing them and the last open PR captures the whole history) but never rewrites their bodies. Root resolution still uses the open-PR list only. - stack-breadcrumb.cjs: reconcile skips writing any member whose state is not 'open' (records them under a new 'frozen' bucket); members without state are treated as open (unit fixtures / open-PR list). - stack-breadcrumb.yml: the reconcile job collects historical members from open PRs' markers, fetches them via pulls.get with state, and passes the union to reconcile; open PRs are tagged state: 'open'. - tests: a merged member is frozen but still listed; a lone open root with only merged descendants still renders (no clear). Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/stack-breadcrumb.cjs | 11 ++++++- .github/scripts/stack-breadcrumb.test.cjs | 37 +++++++++++++++++++++++ .github/workflows/stack-breadcrumb.yml | 37 ++++++++++++++++++++++- 3 files changed, 83 insertions(+), 2 deletions(-) diff --git a/.github/scripts/stack-breadcrumb.cjs b/.github/scripts/stack-breadcrumb.cjs index d7e7e67a..959454a9 100644 --- a/.github/scripts/stack-breadcrumb.cjs +++ b/.github/scripts/stack-breadcrumb.cjs @@ -298,6 +298,7 @@ async function reconcile(root, deps) { const updated = []; const skipped = []; + const frozen = []; const failed = []; for (const number_ of tree.members) { @@ -305,6 +306,14 @@ async function reconcile(root, deps) { if (!pullRequest) { continue; } + // A merged/closed member stays in the tree — so open PRs keep listing it and + // the last open PR captures the full construction history — but its own body + // is frozen (never rewritten). Members without a `state` are treated as open + // (open-PR list and unit fixtures both omit or set it to "open"). + if (pullRequest.state !== undefined && pullRequest.state !== "open") { + frozen.push(number_); + continue; + } const region = renderRegion(tree, number_); const plan = planUpdate(pullRequest, region); if (!plan.changed) { @@ -322,7 +331,7 @@ async function reconcile(root, deps) { } } - return { root, updated, skipped, failed }; + return { root, updated, skipped, frozen, failed }; } module.exports = { diff --git a/.github/scripts/stack-breadcrumb.test.cjs b/.github/scripts/stack-breadcrumb.test.cjs index 15c42292..7a971dd8 100644 --- a/.github/scripts/stack-breadcrumb.test.cjs +++ b/.github/scripts/stack-breadcrumb.test.cjs @@ -276,3 +276,40 @@ test("reconcile: records a failed write and continues", async () => { assert.deepEqual(result.failed, [10]); assert.deepEqual(result.updated, [11]); }); + +test("reconcile: freezes a merged member but keeps it in the open PR's breadcrumb", async () => { + // #10 open (root), #11 merged (tip). #11 stays in the tree so #10 keeps + // listing it, but its own body is never rewritten. + const prs = [ + { ...pr(10, "a", "main"), state: "open" }, + { ...pr(11, "b", "a"), state: "closed" }, + ]; + const calls = []; + const result = await reconcile(10, { + pullRequests: prs, + writeBody: (number_, body) => calls.push([number_, body]), + }); + assert.deepEqual(result.updated, [10]); + assert.deepEqual(result.frozen, [11]); + assert.equal(calls.length, 1); + // #10's breadcrumb still lists the merged #11 and the marker keeps full membership. + assert.match(calls[0][1], /#11/); + assert.match(calls[0][1], /pr=10,11/); +}); + +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. + const prs = [ + { ...pr(10, "a", "main"), state: "open" }, + { ...pr(11, "b", "a"), state: "closed" }, + { ...pr(12, "c", "b"), state: "closed" }, + ]; + const calls = []; + const result = await reconcile(10, { + pullRequests: prs, + writeBody: (number_, body) => calls.push([number_, body]), + }); + assert.deepEqual(result.updated, [10]); + assert.deepEqual(result.frozen, [11, 12]); + assert.match(calls[0][1], /pr=10,11,12/); +}); diff --git a/.github/workflows/stack-breadcrumb.yml b/.github/workflows/stack-breadcrumb.yml index f5e02304..df57becd 100644 --- a/.github/workflows/stack-breadcrumb.yml +++ b/.github/workflows/stack-breadcrumb.yml @@ -145,8 +145,43 @@ jobs: headRefName: pr.head.ref, baseRefName: pr.base.ref, body: pr.body ?? "", + state: "open", })); + // Preserve construction history: pull in stack members recorded in + // existing breadcrumb markers that are no longer open (merged/closed), + // so the tree keeps listing them and the last open PR captures the + // whole stack. Their bodies are frozen (reconcile never rewrites a + // non-open member); they're fetched only for tree structure/rendering. + // Root resolution below still uses the open-PR list only. + const openNumbers = new Set(pullRequests.map((pr) => pr.number)); + const historicalNumbers = new Set(); + for (const pr of pullRequests) { + const marker = stack.parseStackComment(pr.body); + if (!marker) continue; + for (const member of marker.members) { + if (!openNumbers.has(member)) historicalNumbers.add(member); + } + } + const historical = []; + for (const number of historicalNumbers) { + try { + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: number }); + if (pr.head.repo?.full_name !== `${owner}/${repo}`) continue; + historical.push({ + number: pr.number, + title: pr.title, + headRefName: pr.head.ref, + baseRefName: pr.base.ref, + body: pr.body ?? "", + state: pr.state, // "closed" (merged or closed) + }); + } catch (error) { + core.info(`stack-breadcrumb: could not fetch historical PR #${number}: ${String(error)}`); + } + } + const allPullRequests = [...pullRequests, ...historical]; + const writeBody = async (number, body) => { await github.rest.pulls.update({ owner, repo, pull_number: number, body }); }; @@ -207,7 +242,7 @@ jobs: // failure so an inconsistent breadcrumb doesn't pass silently. const failures = []; for (const root of roots) { - const result = await stack.reconcile(root, { pullRequests, writeBody }); + const result = await stack.reconcile(root, { pullRequests: allPullRequests, writeBody }); core.info(`reconciled root #${root}: ${JSON.stringify(result)}`); failures.push(...result.failed); } From f065380fff74e2f4d30eef5691ce88c27e1cc85f Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sat, 4 Jul 2026 13:26:06 -0700 Subject: [PATCH 2/4] fix(ci): harden historical-member fetch in stack breadcrumb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review on #51: - Skip a historical (merged/closed) PR whose head ref collides with an open PR's headRefName — the tree is keyed by head ref, so a reused branch name would corrupt derivation. - Validate marker member numbers (positive integers) before fetching, guarding against a malformed marker. - Fetch the historical members in parallel instead of sequentially (the set is bounded by stack size). Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/stack-breadcrumb.yml | 52 +++++++++++++++++--------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/.github/workflows/stack-breadcrumb.yml b/.github/workflows/stack-breadcrumb.yml index df57becd..bb10cb5b 100644 --- a/.github/workflows/stack-breadcrumb.yml +++ b/.github/workflows/stack-breadcrumb.yml @@ -155,31 +155,47 @@ jobs: // non-open member); they're fetched only for tree structure/rendering. // Root resolution below still uses the open-PR list only. const openNumbers = new Set(pullRequests.map((pr) => pr.number)); + const openHeadRefs = new Set(pullRequests.map((pr) => pr.headRefName)); const historicalNumbers = new Set(); for (const pr of pullRequests) { const marker = stack.parseStackComment(pr.body); if (!marker) continue; for (const member of marker.members) { - if (!openNumbers.has(member)) historicalNumbers.add(member); - } - } - const historical = []; - for (const number of historicalNumbers) { - try { - const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: number }); - if (pr.head.repo?.full_name !== `${owner}/${repo}`) continue; - historical.push({ - number: pr.number, - title: pr.title, - headRefName: pr.head.ref, - baseRefName: pr.base.ref, - body: pr.body ?? "", - state: pr.state, // "closed" (merged or closed) - }); - } catch (error) { - core.info(`stack-breadcrumb: could not fetch historical PR #${number}: ${String(error)}`); + // Guard against a malformed marker (non-positive / non-integer) + // and skip members that are already open. + if (Number.isInteger(member) && member > 0 && !openNumbers.has(member)) { + historicalNumbers.add(member); + } } } + // Fetch in parallel (the set is bounded by stack size). Skip a + // historical PR whose head ref collides with an open PR's — the tree + // is keyed by headRefName, so a reused branch name would corrupt it. + const historical = ( + await Promise.all( + [...historicalNumbers].map(async (number) => { + try { + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: number }); + if (pr.head.repo?.full_name !== `${owner}/${repo}`) return undefined; + if (openHeadRefs.has(pr.head.ref)) { + core.info(`stack-breadcrumb: skipping historical PR #${number}; head ref "${pr.head.ref}" collides with an open PR`); + return undefined; + } + return { + number: pr.number, + title: pr.title, + headRefName: pr.head.ref, + baseRefName: pr.base.ref, + body: pr.body ?? "", + state: pr.state, // "closed" (merged or closed) + }; + } catch (error) { + core.info(`stack-breadcrumb: could not fetch historical PR #${number}: ${String(error)}`); + return undefined; + } + }) + ) + ).filter((pr) => pr !== undefined); const allPullRequests = [...pullRequests, ...historical]; const writeBody = async (number, body) => { From a084a1b3ee8cdce87a83eca80e20c58f99b6b7fd Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sat, 4 Jul 2026 13:39:35 -0700 Subject: [PATCH 3/4] fix(ci): dedupe historical breadcrumb members by head ref Follow-up to the previous hardening: the head-ref collision guard only compared historical PRs against open PRs. Two historical (merged/closed) PRs that reused the same branch name could still collide in the head-ref-keyed tree. Admit a historical PR only if its head ref isn't already claimed by an open OR an earlier historical member. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/stack-breadcrumb.yml | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/.github/workflows/stack-breadcrumb.yml b/.github/workflows/stack-breadcrumb.yml index bb10cb5b..cf2e779f 100644 --- a/.github/workflows/stack-breadcrumb.yml +++ b/.github/workflows/stack-breadcrumb.yml @@ -155,7 +155,6 @@ jobs: // non-open member); they're fetched only for tree structure/rendering. // Root resolution below still uses the open-PR list only. const openNumbers = new Set(pullRequests.map((pr) => pr.number)); - const openHeadRefs = new Set(pullRequests.map((pr) => pr.headRefName)); const historicalNumbers = new Set(); for (const pr of pullRequests) { const marker = stack.parseStackComment(pr.body); @@ -168,19 +167,13 @@ jobs: } } } - // Fetch in parallel (the set is bounded by stack size). Skip a - // historical PR whose head ref collides with an open PR's — the tree - // is keyed by headRefName, so a reused branch name would corrupt it. - const historical = ( + // Fetch in parallel (the set is bounded by stack size); keep same-repo PRs. + const fetched = ( await Promise.all( [...historicalNumbers].map(async (number) => { try { const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: number }); if (pr.head.repo?.full_name !== `${owner}/${repo}`) return undefined; - if (openHeadRefs.has(pr.head.ref)) { - core.info(`stack-breadcrumb: skipping historical PR #${number}; head ref "${pr.head.ref}" collides with an open PR`); - return undefined; - } return { number: pr.number, title: pr.title, @@ -196,6 +189,20 @@ jobs: }) ) ).filter((pr) => pr !== undefined); + // The tree is keyed by headRefName, so admit a historical PR only if + // its head ref is not already claimed — by an open PR OR an earlier + // historical one. A reused branch name would otherwise corrupt tree + // derivation. + const claimedHeadRefs = new Set(pullRequests.map((pr) => pr.headRefName)); + const historical = []; + for (const pr of fetched) { + if (claimedHeadRefs.has(pr.headRefName)) { + core.info(`stack-breadcrumb: skipping historical PR #${pr.number}; head ref "${pr.headRefName}" already claimed by another PR`); + continue; + } + claimedHeadRefs.add(pr.headRefName); + historical.push(pr); + } const allPullRequests = [...pullRequests, ...historical]; const writeBody = async (number, body) => { From 10e31abd9c01074b53b18ff9487c57db48930203 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Sat, 4 Jul 2026 13:49:35 -0700 Subject: [PATCH 4/4] fix(ci): cap historical breadcrumb member fetch defensively The pr= marker lives in a user-editable PR body, so a crafted/malformed list could trigger a large burst of pulls.get calls. Cap the historical fetch at 50 members (with a truncation log) before issuing any API calls. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/stack-breadcrumb.yml | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/.github/workflows/stack-breadcrumb.yml b/.github/workflows/stack-breadcrumb.yml index cf2e779f..b0002670 100644 --- a/.github/workflows/stack-breadcrumb.yml +++ b/.github/workflows/stack-breadcrumb.yml @@ -167,10 +167,18 @@ jobs: } } } - // Fetch in parallel (the set is bounded by stack size); keep same-repo PRs. + // PR bodies are user-editable, so a crafted/malformed marker could + // list many members; cap the historical fetch defensively (with a log). + const MAX_HISTORICAL_MEMBERS = 50; + let historicalList = [...historicalNumbers].toSorted((a, b) => a - b); + if (historicalList.length > MAX_HISTORICAL_MEMBERS) { + core.info(`stack-breadcrumb: marker lists ${historicalList.length} historical members; capping at ${MAX_HISTORICAL_MEMBERS}`); + historicalList = historicalList.slice(0, MAX_HISTORICAL_MEMBERS); + } + // Fetch in parallel (bounded by the cap above); keep same-repo PRs. const fetched = ( await Promise.all( - [...historicalNumbers].map(async (number) => { + historicalList.map(async (number) => { try { const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: number }); if (pr.head.repo?.full_name !== `${owner}/${repo}`) return undefined;