ci: queue PRs in Mergify once verify and Unfret pass - #101
ericlitman wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 📃 Configuration Change RequirementsMergify configuration change
|
|
| batch_size: 1 | ||
| checks_timeout: null | ||
| queue_conditions: [] | ||
| merge_conditions: [] |
| success_conditions: | ||
| - -draft | ||
| - check-success=verify | ||
| - check-success=Unfret |
There was a problem hiding this comment.
If Unfret does not require the independent live verdict, this rule queues any non-draft PR to main once the two checks pass. The PR can then merge while the live test required by the shipping flow is still in progress. Require that verdict before automatic queuing, or leave queuing to the guarded shipping flow.
Knowledge Base Used: Quality assurance and review
There was a problem hiding this comment.
Drafts can't queue: the merge protection requires -draft. AGENTS.md keeps a PR in draft until the installed-candidate evidence is recorded, so marking it ready is the live-verdict gate. That holds only if PRs open as drafts. The live-gate skill in #90 will enforce it by being the step that marks a PR ready. The empty merge_conditions finding is fixed in 635d077.
There was a problem hiding this comment.
That resolves the concern. With PRs opened as drafts and the live-gate skill being the step that marks them ready after installed-candidate evidence, -draft makes the merge protection a live-verdict gate. The added merge_conditions checks also ensure the queued merge commit reruns both verify and Unfret before merging.
There was a problem hiding this comment.
Correction to my reply above: the draft convention doesn't close this gap. Unfret doesn't review drafts (drafting cancels an in-flight review), so a PR has to be non-draft to get its Unfret check, and once it is, this rule queues it as soon as verify and Unfret pass, possibly before the live test. The live verdict needs to be its own required check, for example a commit status on the exact head that the #90 skill posts after the installed-harness test, listed in both merge_conditions and the protection's success_conditions.
There was a problem hiding this comment.
You're right—the draft convention does not close this gap. Unfret cancels its review for drafts, so the PR must become non-draft before check-success=Unfret can pass; at that point, this protection can queue it without any installed-harness/live verdict.
The live-gate result therefore needs to be an independent status/check on the exact tested head, included in both success_conditions and the queue rule's merge_conditions so it is also rerun and required for the queued merge commit. My previous resolution was incorrect; this finding remains valid.
There was a problem hiding this comment.
Refinement, per the review on #104: list live-gate in queue_conditions and the protection's success_conditions only, not in merge_conditions. Mergify validates a temporary queue head that the installed-harness test never sees, so requiring live-gate there would block forever under checks_timeout: null.
There was a problem hiding this comment.
That refinement is correct. live-gate should be added to the queue rule’s queue_conditions and to the protection’s success_conditions, but not to queue_rules[].merge_conditions. Mergify evaluates merge_conditions against the temporary queued merge commit; because the installed-harness test posts its result only for the original PR head, requiring check-success=live-gate there would leave the queued commit unsatisfied and prevent merging indefinitely when checks_timeout: null. The live-gate therefore controls entry into the queue and protection success, while the queued commit itself only reruns the checks that can report on that commit (verify and Unfret).
There was a problem hiding this comment.
Second refinement, from #104's review: putting live-gate only at queue entry still lets a rebuilt queue head merge untested, and non-empty merge_conditions make Mergify create draft batch PRs, which Unfret never reviews. The likely shape: in-place checks (empty merge_conditions, batch_size: 1) plus queue_conditions requiring -draft, verify, Unfret, live-gate, and an up-to-date branch, so the merged tree is the tested tree. Please verify against Mergify's docs before adopting it.
There was a problem hiding this comment.
The docs support the in-place direction, with two qualifications:
merge_conditionsis the merge-time/speculative-check path. A distinct non-empty set can make Mergify validate a temporary draft/batch PR, andbatch_size: 1alone does not guarantee in-place checks. An emptymerge_conditionsis appropriate for the single-step path, but in-place behavior also requires serial operation (max_parallel_checks: 1) andbatch_size: 1(along with the other documented eligibility constraints).queue_conditionscontrol admission and continued queue membership. Requiringcheck-success=live-gatethere means that if the tested SHA changes, the PR no longer satisfies the condition and should leave the queue until a fresh verdict is posted. The status must be attached to the exact commit SHA tested.
Thus, the proposed shape is reasonable:
queue_rules:
- name: default
batch_size: 1
checks_timeout: null
queue_conditions:
- -draft
- check-success=verify
- check-success=Unfret
- check-success=live-gate
merge_conditions: []The queue must also be configured for serial checks (max_parallel_checks: 1), and live-gate should be included in merge_protections[].success_conditions alongside verify and Unfret.
I would not add an invented updated or up-to-date queue condition. Mergify documents GitHub branch protection/rulesets’ strict required-status-checks policy (“Require branches to be up to date before merging”) as the mechanism for that. Since this repository currently has no such protection, enabling it is a separate repository configuration change, not a .mergify.yml condition. Strict protection is also problematic with temporary batch PRs, reinforcing the choice of in-place checks here.
References: queue rules, queue lifecycle, batches and in-place eligibility, and GitHub rulesets.
✅ No blocking findingsWalkthroughAdds a Since the last review
Details
Reviewed the whole pull request. @unfret review · @unfret status · @unfret help |
There was a problem hiding this comment.
🛑 1 blocking finding
🟡 Medium · Queued merge commits can bypass the required checks
Check
| batch_size: 1 | ||
| checks_timeout: null | ||
| queue_conditions: [] | ||
| merge_conditions: [] |
There was a problem hiding this comment.
🟡 Medium · Queued merge commits can bypass the required checks
merge_conditions is empty, so the queue does not require verify or Unfret on its temporary merge commit. If a predecessor lands or main changes after those checks pass on the original PR, the combined commit can fail the newly triggered checks while Mergify still considers queue validation successful and merges it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Tick the box to add this pull request to the merge queue (same as
|
Closes #100
What changed
Adds
.mergify.yml. Mergify now queues a non-draft PR againstmainonceverifyandUnfretpass. It merges with one PR per batch and no checks timeout, and only afterverifyandUnfretpass on the queued merge commit (merge_conditions). open-pstack has no branch protection for Mergify to inject required checks from. This is the same shape as mastra-pilot's config, written standalone because mastra-pilot extends an org-only shared config.@Mergifyio requeueis restricted to writers. Until now there was no config, and #98 merged only after a manual@Mergifyio queue.Unfret rebuttal for head 635d077, finding v1:b39df1497abe8077412bd4881a908445c16acc0aab28368bcfda0d77401dae11
Claim: an outside contributor or bot can open a PR that auto-merges once checks named
verifyandUnfretpass, with no writer acting.Disproof: no one outside the collaborators can open a PR here.
GET repos/ericlitman/open-pstackreturnspull_request_creation_policy: collaborators_only. The only collaborator isericlitman(GET repos/ericlitman/open-pstack/collaborators). The repository has no Dependabot config (.github/holds onlypull_request_template.mdandworkflows). Vulnerability alerts are disabled (GET …/vulnerability-alertsreturns 404), and automated security fixes areenabled: false. Every PR author is therefore a writer, the same trust #98 relied on when its writer posted@Mergifyio queue.Verification
verifyCI runs on it.Live evidence:
Mergify reads its queue rules from
main, so this PR cannot exercise them. The evidence is Mergify's configuration check on this PR, then the next open-pstack PR merging through the queue with no manual queue comment. I'll record that result on #100.🤖 Generated with Claude Code