Repository navigation
Conversation
Stage 2 of the merge-queue rollout (Discussion 208, Option 2), after PR No. 308 added the merge_group CI trigger. MAINTAINING.md and the din-pr-merge / din-pr-review / din-merge-build-test / din-push-develop skills now describe merging via the queue instead of the "branch must be up to date" rule, which the ruleset change drops. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Verification — PR No. 310Reviewed head
|
Files changed (6) — as of
|
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +1/-1 |
| Diff | Replaces the manual up-to-date rationale with queue retesting. |
| Functionality — how & why | Accept PR CI for as-is changes because the queue supplies final integration verification. |
| Diff vs current develop HEAD | No intervening changes at inspected base snapshot. |
| Recommended merge proposal | Merge as-is. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.claude/skills/din-pr-merge/SKILL.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +27/-9 |
| Diff | Changes the gate, enqueues via plain gh pr merge, and documents queue outcomes and rollback. |
| Functionality — how & why | Allows behind branches to enter the queue while retaining approval, CI and conflict gates. |
| Diff vs current develop HEAD | No intervening changes at inspected base snapshot. |
| Recommended merge proposal | Restore the up-to-date requirement in the rollback instructions. |
| Actual merge proposal | Soon |
| Pending proposal | Rollback correction. |
| Local merge conflict | No |
| GitHub merge conflict | No |
.claude/skills/din-pr-review/SKILL.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +2/-2 |
| Diff | Accepts green PR checks in the review-depth rule. |
| Functionality — how & why | Relies on queue integration testing to avoid manual base updates and duplicate verification. |
| Diff vs current develop HEAD | No intervening changes at inspected base snapshot. |
| Recommended merge proposal | Fix the contradictory paragraph at lines 118–120. |
| Actual merge proposal | Soon |
| Pending proposal | Review-evidence correction. |
| Local merge conflict | No |
| GitHub merge conflict | No |
.claude/skills/din-pr-review/templates/verification-comment.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +2/-1 |
| Diff | Updates the CI evidence wording. |
| Functionality — how & why | Keeps the verification template aligned with the queue-based evidence rule. |
| Diff vs current develop HEAD | No intervening changes at inspected base snapshot. |
| Recommended merge proposal | Merge as-is. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.claude/skills/din-push-develop/SKILL.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +1/-1 |
| Diff | Replaces the up-to-date prerequisite with a green queue run. |
| Functionality — how & why | Explains the normal-path checks that an admin direct push bypasses. |
| Diff vs current develop HEAD | No intervening changes at inspected base snapshot. |
| Recommended merge proposal | Merge as-is. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
Developer/MAINTAINING.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +5/-4 |
| Diff | Updates enforcement, evidence, merge path A and local-build guidance. |
| Functionality — how & why | Moves final integration verification to the queue, preserving review and CI requirements. |
| Diff vs current develop HEAD | No intervening changes at inspected base snapshot. |
| Recommended merge proposal | Merge as-is. |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
Verification evidence is recorded in the preceding comment. Outcome comment is deferred until a real merge.
Santiagocetran
left a comment
Santiagocetran
left a comment
There was a problem hiding this comment.
Thanks for putting this together, Umer! This follows the direction we agreed on and should save us the update/reapproval round-trips. PR CI is green, and it’s good to see a real merge queue run passing too.
I spotted two small gaps to address before merging:
- Include restoring the up-to-date requirement in the queue rollback instructions.
- Update the remaining review-skill paragraph that rejects CI when
develophas advanced, so it matches the new queue-based guidance.
Thanks again for carrying the rollout forward!
Reviewed at 4a575b7.
| as usual. | ||
| - **`CI OK` never reports on the queue run** (the queue sits waiting): stop | ||
| and tell Umer. The rollback is turning the merge-queue rule off in the | ||
| `develop` ruleset (Discussion 208). |
There was a problem hiding this comment.
Please include restoring “Require branches to be up to date before merging” in the rollback.
The rollout disables that requirement, so turning off only the merge queue leaves both safeguards disabled. An outdated PR could then merge using green checks that never tested its combination with current develop.
Restoring the up-to-date requirement before allowing direct merges preserves the guarantee we chose in Discussion 208, Option 2.
| **Review depth (MAINTAINING No. 4.2).** Required checks green against an | ||
| up-to-date `develop` count as evidence for anything CI covers: cite the run | ||
| **Review depth (MAINTAINING No. 4.2).** Required checks green on the PR | ||
| count as evidence (the merge queue re-runs them on the latest `develop`) for anything CI covers: cite the run |
There was a problem hiding this comment.
The new wording accepts green PR checks because the queue retests against current develop, but lines 118–120 still say:
If CI on the PR is red, stale against
develop, or never ran, CI isn't evidence
That paragraph then asks for a branch update or local test run, preserving the update/reapproval or duplicate-testing work this change is intended to remove.
Please update it to reject missing, failing, or obsolete checks for the current PR head, while allowing develop to advance and relying on the queue for integration testing.
- din-pr-merge rollback now restores "require branches to be up to date" along with disabling the merge queue (review item 1). - din-pr-review no longer rejects green PR CI just because the branch is behind develop; the merge queue re-tests on the latest develop (review item 2). - Queue with the GraphQL enqueuePullRequest mutation instead of gh pr merge, which fails here with "Auto merge is not allowed for this repository" (seen enqueueing PR No. 296). MAINTAINING.md path A now says Merge when ready only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks, Santiago. Both items are addressed in
One more change from the live rollout: |
|
Reviewed against Claimed: Docs and skills only; no code or workflow changes. Verified: Claimed: Verified: Running that exact grep against the PR worktree returns 3 lines: Claimed: The enqueue mutation in Verified: Claimed: No overlap between this PR's files and what Verified: No |
Files changed (6) — as of
|
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +9/-5 |
| Diff (what exactly is in this PR) | No. 2: removes "branch must be up to date with develop before merging", replaces with a description of the merge queue and what it tests. No. 4.2 evidence wording: "green against an up-to-date develop" → "green on the PR" (the queue re-runs them). No. 4.4 Path A: "CI is green, the branch is up to date, and the reviewer merges with a merge commit" → "CI is green, and the reviewer clicks Merge when ready to add the PR to the merge queue; the queue re-runs CI OK on the latest develop and lands it with a merge commit." Last paragraph: removes "branch has to be up to date" → "merge queue runs it again on the exact commit that lands." |
| Functionality — how & why | How: The four hunks together remove every instance of the old "branch must be up to date" gate from the process document and replace them with the merge-queue equivalent. Why: PR No. 308 enabled the merge_group CI trigger and the develop ruleset was updated to require the merge queue and turn off the up-to-date requirement. MAINTAINING.md previously described a gate that no longer exists on the ruleset, making the process doc wrong for any reviewer following it. |
Diff vs current develop HEAD |
None — MAINTAINING.md not touched on develop since the merge base |
| Recommended merge proposal | Merged as-is |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.claude/skills/din-pr-merge/SKILL.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +43/-5 |
| Diff (what exactly is in this PR) | frontmatter description updated. Path A gate table: "CI OK, branch up to date" → "CI OK, then merge queue's own CI OK run". A2: mergeStateStatus CLEAN → not DIRTY (behind is fine; explicitly says don't run gh pr update-branch). A3: replaces gh pr merge --merge with a two-step GraphQL enqueuePullRequest mutation; adds merged/dropped/stalled handling; adds rollback instructions (turn queue rule off and up-to-date rule back on together). |
| Functionality — how & why | How: The most substantive change. gh pr merge with a merge-commit strategy fails when auto-merge is disabled on the repo, so A3 now uses the GraphQL mutation that mirrors the Merge when ready button. The DIRTY-only gate in A2 stops reviewers from running gh pr update-branch (which would reset the approval and make them the last pusher). The dropped/stalled paths close the loop on what to do when the queue doesn't finish. Why: Without this, any maintainer following the old A3 would hit "Auto merge is not allowed for this repository" and either use --admin (wrong) or be stuck. |
Diff vs current develop HEAD |
None — din-pr-merge SKILL.md not touched on develop since the merge base |
| Recommended merge proposal | Merged as-is |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.claude/skills/din-pr-review/SKILL.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +12/-9 |
| Diff (what exactly is in this PR) | Section 2: "green against an up-to-date develop" → "green on the PR (merge queue re-runs them on the latest develop)". Removes "stale against develop" as a reason to reject CI as evidence. Adds: don't ask the author to update the branch if it's merely behind; only ask when there's a real conflict or the PR depends on something develop gained. |
| Functionality — how & why | How: Aligns the reviewer's evidence standard with the new queue: a PR that is behind develop is still valid evidence because the queue will test it on top of the latest develop before it lands. Removes the old "stale against develop → CI not evidence" path, which would have been wrong under the queue. Why: A reviewer following the old wording would incorrectly reject green CI on a PR that is simply behind, or ask for a develop merge that would reset the approval. |
Diff vs current develop HEAD |
None — din-pr-review SKILL.md not touched on develop since the merge base |
| Recommended merge proposal | Merged as-is |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.claude/skills/din-pr-review/templates/verification-comment.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +2/-1 |
| Diff (what exactly is in this PR) | Template wording: "green against an up-to-date develop" → "green on the PR; the merge queue re-runs it on the latest develop". |
| Functionality — how & why | How: One-line wording update to the verification comment template so the template text the reviewer copies matches the new evidence standard. Why: Consistency — the template would otherwise describe a standard that contradicts MAINTAINING.md No. 4.2 and the din-pr-review skill. |
Diff vs current develop HEAD |
None — not touched on develop since the merge base |
| Recommended merge proposal | Merged as-is |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.claude/skills/din-merge-build-test/SKILL.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +1/-1 |
| Diff (what exactly is in this PR) | "As-is" row in the build evidence table: "ruleset requires the branch to be up to date, so that run tests exactly what lands" → "merge queue re-runs it on the exact commit that lands". |
| Functionality — how & why | How: One-line wording update removing the up-to-date gate reference. Why: The ruleset no longer requires the branch to be up to date; the merge queue provides the equivalent guarantee. The cell previously cited a no-longer-enforced requirement. |
Diff vs current develop HEAD |
None — not touched on develop since the merge base |
| Recommended merge proposal | Merged as-is |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
.claude/skills/din-push-develop/SKILL.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +1/-1 |
| Diff (what exactly is in this PR) | Opening sentence: "approval, CI OK and an up-to-date branch" → "approval, CI OK and a green merge-queue run". |
| Functionality — how & why | How: One-line description update to the path A gate. Why: Same as above — aligns the description of what the ruleset requires with what it actually enforces now. |
Diff vs current develop HEAD |
None — not touched on develop since the merge base |
| Recommended merge proposal | Merged as-is |
| Actual merge proposal | Soon |
| Pending proposal | None |
| Local merge conflict | No |
| GitHub merge conflict | No |
Verification
Deep verification detail in the separate verification comment. Summary: all 4 CI checks pass at run 41475194544. The enqueuePullRequest mutation in A3 is confirmed live — it was the call that queued PR No. 296, which merged as 8f291c11 via merge_group run 41468689673 (conclusion: success). Docs and skills only; no code or workflow changes.
Local vs. GitHub agree: git merge-tree --write-tree exit 0 (no file list) matches GitHub mergeable: MERGEABLE.
robertocarlous
left a comment
robertocarlous
left a comment
There was a problem hiding this comment.
Verified at 079cacb: docs/skills only, all 4 CI checks green, clean merge, enqueue mutation confirmed live via PR No. 296 merge_group run 41468689673.
Summary
Stage 2 of the merge-queue rollout agreed in Discussion 208 (Option 2). PR No. 308 added the
merge_groupCI trigger, and thedevelopruleset now requires the merge queue. This PR updates the process docs and skills to match.Developer/MAINTAINING.md:din-pr-merge:CLEANorgh pr update-branch. OnlyDIRTY(a conflict) blocks.enqueuePullRequestmutation.gh pr mergefails here with "Auto merge is not allowed for this repository", because auto-merge is off.merge_grouprun and what to do when a PR is dropped from the queue.din-pr-review:developno longer makes CI "stale" evidence.din-merge-build-test,din-push-develop: one-line wording updates.Ruleset change (already applied)
On the
developruleset, "Require merge queue" is on (merge commit, ALLGREEN, 5 PRs built at once, minimum 1, no wait, 60-minute timeout forCI OK) and "Require branches to be up to date" is off. PR No. 296 was the first PR through. The queue's CI (run 41468689673) passedCI OK, and the queue merged it as8f291c11.Verification
grep -rn -i "up to date\|update-branch" Developer/MAINTAINING.md .claude/skills/leaves only the new queue wording, the rollback note, and one unrelated localmerge-basecheck indin-merge-build-test.din-pr-mergeA3 is the call that queued PR No. 296.Part of Discussion 208.
🤖 Generated with Claude Code