feat(stack): hand the stack's member list to GitHub's native UI - #1851
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two moderate cleanup issues and two documentation nits remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
This PR uses GitHub’s native Stacks UI for registered stacks, removing legacy comments while preserving fallback behavior.
Changes:
- Deletes legacy comments after successful native registration.
- Retains comment updates when registration is unavailable.
- Updates documentation and adds unit/integration coverage.
| File | Summary |
|---|---|
skills/mergify-stack/SKILL.md |
Documents native and fallback stack-comment behavior. |
crates/mergify-stack/src/stack_comment.rs |
Corrects marker documentation; one nit requests clearer wording. |
crates/mergify-stack/src/lib.rs |
Updates module documentation; one nit notes registered cleanup also uses the recognizer. |
crates/mergify-stack/src/comment_upsert.rs |
Adds comment deletion support and tests. |
crates/mergify-stack/src/commands/push.rs |
Coordinates native cleanup and fallback updates; two moderate issues remain around single-member cleanup and closed PR handling. |
crates/mergify-cli/tests/stack_push_github_native.rs |
Adds integration coverage for native stack behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Merge Protections🟢 All 6 merge protections satisfied — ready to merge. Show 6 satisfied protections🟢 🤖 Continuous Integration
🟢 👀 Review Requirements
🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 🔎 Reviews
🟢 📕 PR description
🟢 🚦 Auto-queueWhen all merge protections are satisfied, this pull request will be queued automatically. |
`mergify stack push` posted a sticky "This pull request is part of a Mergify stack" comment on every member, carrying a table of the whole stack. GitHub's native Stacks UI now renders that list on the pull request page itself, so the comment is a second copy of what the reader is already looking at — one they have to reconcile against the first whenever the two disagree. So a push whose stack GitHub registered posts no table, and deletes the one an earlier push left on each open member. **Keyed off the registration, not off the flag.** This is the same argument as the `Depends-On:` header one step up, and it gets the same treatment for the same reason: `native_stack::register` is explicitly allowed to do nothing — an older GitHub Enterprise, a repo without the feature, a chain GitHub will not accept, or `--no-github-native`. Drop the comment unconditionally and those pushes end up with no member list anywhere, which is a worse place than where they started. A push that did not register keeps posting and updating the table exactly as before. The settling moves to the end of the push (new step 13), next to the header restore it mirrors, because the registration's outcome is only known there. **Comments already on live stacks are deleted, not left or rewritten.** Leaving them is the tempting option and the wrong one: nothing refreshes a table we no longer write, so each would sit frozen at the membership of the last pre-native push, under a live list that keeps moving — stale next to accurate is worse than absent next to accurate. A one-time rewrite to a tombstone would leave permanent noise on every member of every stack ever pushed. Deleting costs nothing recoverable: the comment was ours, bot-authored, and carried no reply thread. The scan that finds it is the GET the upsert already did on every push, so this is not a new request — it is the same one, ending in a DELETE instead of a PATCH. Merged members are skipped, as the upsert always skipped them. Only the fallback upsert is gated on the stack having more than one pull request, where the count saves a genuinely one-change stack a GET that can only come back empty. The removal is not: a stack whose members merge down to a single open pull request keeps its registration, so that last member is still carrying the table it was given while the stack had several — exactly the frozen list this is taking down. The gate costs the removal nothing anyway, since GitHub rejects a stack below two members and an unregistered stack never reaches the removal. The delete is best-effort: it changes nothing about how the stack merges, so a push that has already done everything else reports it and retries on the next push rather than failing. That is the opposite policy from the header restore, which is fatal precisely because an unregistered, unchained stack merges out of order. **The revision-history comment is untouched.** GitHub renders nothing like it, which is what the sticky-comment surface exists for now. Also corrects a stale claim in `stack_comment`'s docstring: it said `mergify stack checkout` rebuilds a stack from the `<!-- mergify-stack-data: -->` JSON marker. It does not, and nothing in this repo reads that marker — `checkout` discovers a stack by chaining each pull request's `head.ref` to the next one's `base.ref`, which has the advantage of working on stacks this CLI never touched. The marker's readers are out of tree, so its wire shape stays pinned. MRGFY-9496 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Change-Id: I4d272321c1cfcb3b836a1fe1ac2ef0ce463ec550
db4e03d to
2e66ef5
Compare
Revision history
|
|
Re-pushed: Two things, one of them a real hole in what this PR promises: The comment removal no longer depends on the member count. The gate was New test I have deliberately not covered a stack that shrinks and loses its registration (GitHub dissolves a 2-member stack that drops to one). Finding that comment means a GET on every single-change push for ever, to clean an artifact only pre-native stacks can carry. The reasoning is in the code beside the gate; the gap already existed on Typo checker. |
Merge Queue Status
This pull request spent 56 seconds in the queue, including 1 second running CI. Required conditions to merge
|
mergify stack pushposted a sticky "This pull request is part of aMergify stack" comment on every member, carrying a table of the whole
stack. GitHub's native Stacks UI now renders that list on the pull
request page itself, so the comment is a second copy of what the reader
is already looking at — one they have to reconcile against the first
whenever the two disagree.
So a push whose stack GitHub registered posts no table, and deletes the
one an earlier push left on each open member.
Keyed off the registration, not off the flag. This is the same
argument as the
Depends-On:header one step up, and it gets the sametreatment for the same reason:
native_stack::registeris explicitlyallowed to do nothing — an older GitHub Enterprise, a repo without the
feature, a chain GitHub will not accept, or
--no-github-native. Dropthe comment unconditionally and those pushes end up with no member list
anywhere, which is a worse place than where they started. A push that
did not register keeps posting and updating the table exactly as
before. The settling moves to the end of the push (new step 13), next
to the header restore it mirrors, because the registration's outcome is
only known there.
Comments already on live stacks are deleted, not left or rewritten.
Leaving them is the tempting option and the wrong one: nothing
refreshes a table we no longer write, so each would sit frozen at the
membership of the last pre-native push, under a live list that keeps
moving — stale next to accurate is worse than absent next to accurate.
A one-time rewrite to a tombstone would leave permanent noise on every
member of every stack ever pushed. Deleting costs nothing recoverable:
the comment was ours, bot-authored, and carried no reply thread. The
scan that finds it is the GET the upsert already did on every push, so
this is not a new request — it is the same one, ending in a DELETE
instead of a PATCH. Merged members are skipped, as the upsert always
skipped them.
Only the fallback upsert is gated on the stack having more than one
pull request, where the count saves a genuinely one-change stack a GET
that can only come back empty. The removal is not: a stack whose
members merge down to a single open pull request keeps its
registration, so that last member is still carrying the table it was
given while the stack had several — exactly the frozen list this is
taking down. The gate costs the removal nothing anyway, since GitHub
rejects a stack below two members and an unregistered stack never
reaches the removal.
The delete is best-effort: it changes nothing about how the stack
merges, so a push that has already done everything else reports it and
retries on the next push rather than failing. That is the opposite
policy from the header restore, which is fatal precisely because an
unregistered, unchained stack merges out of order.
The revision-history comment is untouched. GitHub renders nothing
like it, which is what the sticky-comment surface exists for now.
Also corrects a stale claim in
stack_comment's docstring: it saidmergify stack checkoutrebuilds a stack from the<!-- mergify-stack-data: -->JSON marker. It does not, and nothing inthis repo reads that marker —
checkoutdiscovers a stack by chainingeach pull request's
head.refto the next one'sbase.ref, which hasthe advantage of working on stacks this CLI never touched. The marker's
readers are out of tree, so its wire shape stays pinned.
MRGFY-9496
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com