Collect staging branches for pull requests opened from forks - #9335
Merged
Conversation
The reconciliation sweep only ever considered pull requests whose head branch lived in this repository. That guard is right for the head branch -- a contributor's fork is not ours to write to -- but it was applied to the whole pull request, so the staging branch created for it was abandoned too, even though that branch is always in this repository. Most advisory improvements arrive from a fork, so the sweep was cleaning up after the minority and leaving the majority behind. 965 of the 1,463 staging branches currently on the repository are in that state: their pull request is closed or merged, their base is their own staging branch, and nothing will ever collect them. The sweep has already exhausted the in-repository population, so it now inspects every branch and reconciles none. Treat a fork head as a reason to leave the head branch alone rather than a reason to skip the pull request, and delete the staging branch it targeted. Every existing gate still applies: the pull request must be closed, its base must be this repository, its base must be its own staging branch, and it must touch advisories/. The branch is confirmed not to be the base of any open pull request immediately before deletion, and the staging branch name is re-validated against the pull request number inside the delete path. Because deleting refs is destructive, a sweep now reconciles at most MAX_RECONCILE_PER_RUN branches and defers the rest to the next scheduled sweep, which bounds both blast radius and runtime per run without needing anyone to drive the backlog down by hand. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: daca0885-6779-4adb-bb02-ff6920d73ab3
Two problems in the review of the previous commit. The open pull request cache never took effect. open_pr_targeting_base was only ever called from a command substitution, so the global it assigned was discarded with the subshell and the full paginated listing was refetched for every branch. At 400 branches a run that is roughly 1,600 wasted requests every ten minutes, which would exhaust the token budget, fail every remaining pull request in the run once it did, and leak a temporary file per branch because cleanup never saw the name. Load the cache from delete_staging_branch, which runs in the caller's shell, and make the lookup treat an unloaded cache as a failed lookup rather than as "nothing is using this branch". The lookup also only considered pull requests that use a branch as their base. A staging branch is normally a base, but the curation flow can open a pull request with one as its head, and that use has to keep the branch alive too. Index head refs as well, restricted to refs in this repository so that a fork branch of the same name cannot mask a deletable staging branch. Both refs are already in the listing, so this costs no extra requests. No open pull request is in that state today; the guard is there so the flow cannot grow into the gap. Also handle a return code of 3 from delete_branch instead of collapsing it into a failure, so adding a SHA guard here later cannot turn a legitimate skip into a red run, and label the summary row "attempted", which is what it counts now that a run can be capped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: daca0885-6779-4adb-bb02-ff6920d73ab3
record_skip appends to the file the job summary is built from, but every caller runs under "if ! process_pr", which disables set -e for the function and everything it calls. The "return 0" that followed each call overwrote the append's exit status, so a failed write left the run green while silently dropping the branch from the summary. Two of the four call sites are added by this branch, and the summary is the only record of which branches were left in place, so a skip that cannot be recorded is now surfaced as a failure instead of being lost. Nothing has been deleted at any of these points, so failing there cannot leave a branch half-processed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: daca0885-6779-4adb-bb02-ff6920d73ab3
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical race and coverage gaps could allow deletion of staging branches used by open pull requests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
.github/workflows/delete_staging_and_head_branches_writer.yaml — The early return makes this safety cache stale for the rest of a sweep. Per-PR runs are explicitly… |
What changed in this PR
Extends staging-branch cleanup to closed pull requests originating from forks.
Changes:
- Adds cached open-PR ref protection.
- Includes fork-originated pull requests in reconciliation.
- Caps scheduled sweeps at 400 targets.
| File | Description |
|---|---|
.github/workflows/delete_staging_and_head_branches_writer.yaml |
Extends and bounds staging-branch cleanup. |
Suppressed comments (1)
.github/workflows/delete_staging_and_head_branches_writer.yaml:458
- Only fork-headed pull requests use the new open-ref guard. The in-repository paths below still delete
base_refthroughdelete_branchdirectly, including when a curation pull request is currently using that staging branch as its local head—the exact case indexed above. Apply the same open-base/head protection to every staging-branch deletion while preserving the local head SHA and deletion-order checks.
return 1
fi
if ! grep -qx 'true' <<<"${advisory_file_pages}"; then
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| load_open_pr_refs() { | ||
| local file raw | ||
|
|
||
| [[ -z "${OPEN_PR_REFS_FILE}" ]] || return 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The scheduled sweep now inspects every staging branch and reconciles none:
That is not the backlog being drained. The sweep has exhausted the only population it was ever capable of seeing.
process_prbailed out entirely on any pull request whose head branch was on a fork:That guard is correct for the head branch — a contributor's fork is not ours to write to. But it was applied to the whole pull request, so the staging branch was abandoned too, even though the staging branch always lives in this repository and is exactly what the sweep exists to collect.
triage_chunkhad the matching filter (select(.headRepository.nameWithOwner == $repo)), which is why triage surfaced 4 candidates rather than hundreds.Most advisory improvements arrive from a fork, so the workflow was cleaning up after the minority and permanently leaking the majority.
Current state of the repository
All 1,463 staging branches resolved against the API:
main(curation topology)The 965 are 646 closed and 319 merged.
Change
A fork head now means "leave the head branch alone", not "skip the pull request". The staging branch it targeted is collected by a new
delete_staging_branch.Every existing gate still applies, in the same order: the pull request must be closed, its base must be this repository, its base must still be the branch observed in the branch listing,
is_staging_branch_for_prmust confirm the base encodes this pull request's number, and it must modifyadvisories/.delete_staging_branchre-validates the branch against the pull request number, and confirms no open pull request is using the ref before deleting.Net effect: for pull request N, the only ref that can be deleted is
<single-segment-prefix>/advisory-improvement-N.mainis not reachable.Because deleting refs is destructive, a sweep now reconciles at most
MAX_RECONCILE_PER_RUN(400) branches and defers the rest to the next scheduled sweep, so no single run has an unbounded blast radius or runtime. The backlog drains automatically over ~3 sweeps.Validation
Live dry run of the real selection logic against this repository (read-only —
collect_reconciliation_targetsonly lists and triages):The 965 selected match, exactly and with zero difference, a set derived independently by resolving every branch through a separate GraphQL pass. Cross-checked against the live API:
71 assertions pass against the real script body extracted from this workflow, covering fork paths (closed, open, deleted fork, unusable head SHA, base retargeted at
main, base in another repository, retargeted since the listing, does not touchadvisories/), the open-PR cache contract, and regressions on the in-repository path (head deleted before base,head_ref == base_ref, bad head SHA still fails, reused head still returns 3 and preserves the base).actionlint,bash -n, andshellcheckare clean.Review findings fixed in the second commit
A code review caught that the open-PR cache never took effect:
open_pr_targeting_basewas only called from a command substitution, so the global it assigned died with the subshell and the full paginated listing was refetched per branch — roughly 1,600 wasted requests per run, which would exhaust the token budget, fail every remaining pull request once it did, and leak a temp file per branch. The load now happens indelete_staging_branch, which runs in the caller's shell, and an unloaded cache is treated as a failed lookup rather than as "nothing is using this branch". Regression test asserts the listing is fetched once per run.A security review found no vulnerabilities, and separately confirmed the same caching bug. It flagged one asymmetry: the check only considered pull requests using a branch as their base, not as their head. No open pull request is in that state today, but the curation flow can open a staging-branch →
mainpull request, so head refs are now indexed too — restricted to refs in this repository so a fork branch of the same name cannot mask a deletable staging branch. Both refs are already in the listing, so this costs no extra requests.Rollout
No new permissions, no new triggers. First scheduled sweep after merge will reconcile 400 and report
Deferred to the next sweep | 565in the job summary; the backlog reaches zero after the third sweep, then the steady state is a handful per sweep as pull requests close.Watch the first run's summary for a non-zero "Left in place" or "Pull request failures". Reverting restores the previous behavior; no branch deletion is irreversible in a way that loses data, since merged work is in
mainand closed pull requests retain their diffs.Still open, deliberately not addressed here
mainrather than its own staging branch — a different topology that this workflow was not written to reconcile.