From 4a575b74d47cfdc8c0ff8f71be44a83c271b6620 Mon Sep 17 00:00:00 2001 From: umermjd11 Date: Mon, 2 Nov 2026 19:32:22 +0500 Subject: [PATCH 1/2] docs(maintaining,skills): land develop PRs through the merge queue 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 --- .claude/skills/din-merge-build-test/SKILL.md | 2 +- .claude/skills/din-pr-merge/SKILL.md | 36 ++++++++++++++----- .claude/skills/din-pr-review/SKILL.md | 4 +-- .../templates/verification-comment.md | 3 +- .claude/skills/din-push-develop/SKILL.md | 2 +- Developer/MAINTAINING.md | 9 ++--- 6 files changed, 38 insertions(+), 18 deletions(-) diff --git a/.claude/skills/din-merge-build-test/SKILL.md b/.claude/skills/din-merge-build-test/SKILL.md index 116944e..4acfacb 100644 --- a/.claude/skills/din-merge-build-test/SKILL.md +++ b/.claude/skills/din-merge-build-test/SKILL.md @@ -21,7 +21,7 @@ setup is [`.claude/scripts/README.md`](../../scripts/README.md). | Situation | Build/test evidence | |---|---| -| PR merges **as-is** (every "Pending proposal" is `None`) | The PR's green `CI OK`. CI runs the full pipeline on GitHub's merge of the PR into `develop` (via_ir `forge build`, size gate, `forge test`, hardhat, pytest, doc links), and the ruleset requires the branch to be up to date, so that run tests exactly what lands. **Don't run this skill.** | +| PR merges **as-is** (every "Pending proposal" is `None`) | The PR's green `CI OK`. CI runs the full pipeline on GitHub's merge of the PR into `develop` (via_ir `forge build`, size gate, `forge test`, hardhat, pytest, doc links), and the merge queue re-runs it on the exact commit that lands. **Don't run this skill.** | | Fixes applied, **path A** | This skill, in the review worktree, **before** the fixes are pushed. After the push, the PR's CI re-runs and is the gate. | | Fixes applied, **path B** | This skill, in the `develop` checkout. There is no PR CI for the deviation, so this run plus `din-push-develop`'s precheck are the gate. | diff --git a/.claude/skills/din-pr-merge/SKILL.md b/.claude/skills/din-pr-merge/SKILL.md index 06c15f0..c019a03 100644 --- a/.claude/skills/din-pr-merge/SKILL.md +++ b/.claude/skills/din-pr-merge/SKILL.md @@ -1,6 +1,6 @@ --- name: din-pr-merge -description: Use when actually merging a PR into InfiniteZeroFoundation/DevNet's `develop` — "merge PR #N", "merge this into develop", or any request to land a PR after it's been reviewed. Any maintainer who isn't the PR's author or a co-author can run it (checked first with `.claude/scripts/din_peer_check.py`). Path A (the default, per Developer/MAINTAINING.md No. 4.4): push any review fixes onto the PR branch, then merge on GitHub with a merge commit once the approval, `CI OK` and up-to-date gates pass. Path B (Umer only per MAINTAINING No. 4.4, exceptional): the local stash-merge-pop flow plus a deviation commit, pushed through `din-push-develop` with the admin bypass. Either way it ends with the merger watching `develop` CI and posting the outcome comment (`outcome-comment.md`). Runs after `din-pr-review`. Not for reviewing a PR (use `din-pr-review`) and not for generic git merges outside this repo's PR workflow. +description: Use when actually merging a PR into InfiniteZeroFoundation/DevNet's `develop` — "merge PR #N", "merge this into develop", or any request to land a PR after it's been reviewed. Any maintainer who isn't the PR's author or a co-author can run it (checked first with `.claude/scripts/din_peer_check.py`). Path A (the default, per Developer/MAINTAINING.md No. 4.4): push any review fixes onto the PR branch, then add it to the `develop` merge queue once the approval and `CI OK` gates pass; the queue re-runs CI on the latest `develop` and lands it with a merge commit. Path B (Umer only per MAINTAINING No. 4.4, exceptional): the local stash-merge-pop flow plus a deviation commit, pushed through `din-push-develop` with the admin bypass. Either way it ends with the merger watching `develop` CI and posting the outcome comment (`outcome-comment.md`). Runs after `din-pr-review`. Not for reviewing a PR (use `din-pr-review`) and not for generic git merges outside this repo's PR workflow. --- # DIN DevNet PR merge @@ -20,7 +20,7 @@ setup is [`.claude/scripts/README.md`](../../scripts/README.md). | Who | Any maintainer who isn't the author or a co-author | Umer only (MAINTAINING No. 4.4; `din_peer_check.py path-b`) | | When | The default, for every PR | Exceptional: the PR needs adapting against a `develop` that has moved (renames, ABI regeneration, docs drift) and pushing fixes onto the PR branch isn't practical | | Fixes | Pushed onto the PR branch (or a small follow-up PR); the last-push rule sends the final approval to someone else | A separate deviation commit on top of the merge, on `develop` | -| Gate | The `develop` ruleset: 1 approval of the latest push, `CI OK`, branch up to date | The admin bypass; log a one-line reason in [Discussion 170](https://github.com/InfiniteZeroFoundation/DevNet/discussions/170) | +| Gate | The `develop` ruleset: 1 approval of the latest push, `CI OK`, then the merge queue's own `CI OK` run | The admin bypass; log a one-line reason in [Discussion 170](https://github.com/InfiniteZeroFoundation/DevNet/discussions/170) | Use path A unless the operator explicitly chooses B and is Umer. @@ -97,10 +97,10 @@ All of these must hold: - `isDraft` is false. - An approval from an eligible peer is **on the current head SHA**. - `CI OK` is green on that head. -- `mergeStateStatus` is `CLEAN`. `BEHIND` means the branch isn't up to date: - ask the author to update it, or run `gh pr update-branch `. Updating it - makes the operator the last pusher, so the approval then has to come from - someone else. +- `mergeStateStatus` isn't `DIRTY`. `DIRTY` means a merge conflict with + `develop`: ask the author to resolve it. Being behind `develop` is fine — the merge queue tests the PR on + top of the latest `develop`, so don't run `gh pr update-branch` (it would + make the operator the last pusher and reset the approval). - **`foundry/src/**` touched?** The second sign-off (MAINTAINING No. 4.2) is on the PR, as an approval or a comment from @umeradl, @abrahamnash or a security reviewer they named. @@ -111,14 +111,32 @@ If any gate fails, report which one and stop. Don't use `--admin`. ### A3. Merge -On the operator's go-ahead: +On the operator's go-ahead, add the PR to the merge queue. `develop` +requires the queue, so `gh pr merge` queues it rather than merging directly, +and the queue picks the merge method (merge commit). Don't pass `--admin`, +which would skip the queue. ```bash -gh pr merge -R $R --merge +gh pr merge -R $R +``` + +The queue builds a temporary `gh-readonly-queue/develop/...` branch (latest +`develop` + any PRs ahead + this PR) and runs `CI OK` on it, about 8 minutes. +Watch it in the background rather than polling: + +```bash +gh run list -R $R --event merge_group --limit 5 # the queue's CI run gh pr view -R $R --json state,mergedAt,mergeCommit --jq '{state, mergedAt, sha: .mergeCommit.oid}' ``` -Then the outcome steps: **C1–C2** below. +- **Merged:** `state` is `MERGED`; continue with **C1–C2** below. +- **Dropped from the queue** (its CI run failed, or a PR ahead of it broke the + group): `develop` is untouched. Read the failed run, tell the author, and + re-queue once it's fixed. A fix pushed to the branch needs a fresh approval + 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). --- diff --git a/.claude/skills/din-pr-review/SKILL.md b/.claude/skills/din-pr-review/SKILL.md index 5cc08ab..c95b2c0 100644 --- a/.claude/skills/din-pr-review/SKILL.md +++ b/.claude/skills/din-pr-review/SKILL.md @@ -103,8 +103,8 @@ list` first — the review may already be set up. ## 2. What to run locally — and what CI already covers -**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 (`gh pr checks `, link the job) instead of re-running it. **Reproduce locally only what CI doesn't cover:** diff --git a/.claude/skills/din-pr-review/templates/verification-comment.md b/.claude/skills/din-pr-review/templates/verification-comment.md index d6efb3a..9dc10f7 100644 --- a/.claude/skills/din-pr-review/templates/verification-comment.md +++ b/.claude/skills/din-pr-review/templates/verification-comment.md @@ -2,7 +2,8 @@ Posted first, before the merge-proposal table. Goal: a "Claimed / Verified" pair for every checkable claim in the PR. A claim is verified by linking the -CI job that covers it (green against an up-to-date `develop`), or by running it +CI job that covers it (green on the PR; the merge queue re-runs it on the +latest `develop`), or by running it locally where `Developer/MAINTAINING.md` No. 4.2 requires that: storage layout and upgrade safety in `foundry/src/**`, deploy/upgrade scripts, `dincli` flows against a live chain or IPFS, and any behaviour no test exercises. diff --git a/.claude/skills/din-push-develop/SKILL.md b/.claude/skills/din-push-develop/SKILL.md index 20bf5cf..ca3e3dc 100644 --- a/.claude/skills/din-push-develop/SKILL.md +++ b/.claude/skills/din-push-develop/SKILL.md @@ -5,7 +5,7 @@ description: Use whenever an admin pushes directly to `develop` on InfiniteZeroF # Push to develop, CI-checked -Normal merges land through a PR (`din-pr-merge` path A), where the `develop` ruleset requires an approval, `CI OK` and an up-to-date branch. A direct push skips all of that, so it's reserved for admins using the bypass: path-B local merges and emergency fixes (Developer/MAINTAINING.md No. 4.4). This skill puts the missing checks back around such a push: a local CI mirror before it, and a watch on the GitHub `push` run after it. +Normal merges land through a PR (`din-pr-merge` path A), where the `develop` ruleset requires an approval, `CI OK` and a green merge-queue run. A direct push skips all of that, so it's reserved for admins using the bypass: path-B local merges and emergency fixes (Developer/MAINTAINING.md No. 4.4). This skill puts the missing checks back around such a push: a local CI mirror before it, and a watch on the GitHub `push` run after it. The rule ([Developer/MAINTAINING.md](../../../Developer/MAINTAINING.md) No. 4.6, [discussion No. 170](https://github.com/InfiniteZeroFoundation/DevNet/discussions/170)): whoever pushes or merges owns that push's CI run and makes sure it's fixed the same working day. If a red run is left unaddressed for ~6h and more commits land on top, the other maintainers open an incident thread in No. 170. diff --git a/Developer/MAINTAINING.md b/Developer/MAINTAINING.md index 73e4de5..336e9c1 100644 --- a/Developer/MAINTAINING.md +++ b/Developer/MAINTAINING.md @@ -30,7 +30,8 @@ Umer's own PRs (opened from the `umermjd11` fork) follow the same rule: one of t - 1 approving review from someone other than the author. - **The most recent push must be approved by someone other than the person who pushed it.** If a reviewer pushes fixes onto your PR, they can't approve it themselves any more. Someone else has to. - Stale approvals are dismissed when new commits are pushed. -- The `CI OK` check must pass, and the branch must be **up to date with `develop`** before merging. +- The `CI OK` check must pass on the PR. +- PRs land through the **merge queue**. The queue tests the PR merged onto the latest `develop` (plus any PRs queued ahead of it) and merges only if `CI OK` passes there too. The branch doesn't have to be up to date by hand, so nobody clicks **Update branch** and approvals aren't reset by it. - Merge commits only. Squash and rebase merges are turned off, so authorship and history survive. - No force-pushes, no deleting the branch. - Only the repository admin role can bypass these rules ([No. 4.4](#44-merge-paths)). @@ -64,7 +65,7 @@ CODEOWNERS requests reviewers when you open the PR. Check that at least one requ - **Any PR touching `foundry/src/**`** (contract logic, storage layout, upgrade paths) needs the peer's review **plus a second sign-off** from @umeradl or @abrahamnash, or from a security reviewer they route it to. Mechanism changes also need the [mechanism sign-off](#mechanism-sign-off) below. - Docs-only, test-only, Python and tooling PRs need one peer. -**What counts as evidence.** Required checks that are green against an up-to-date `develop` count as evidence for anything CI covers. The reviewer cites the CI run and doesn't re-run it. **Local reproduction is required only for what CI doesn't cover:** +**What counts as evidence.** Required checks that are green on the PR count as evidence for anything CI covers. The merge queue re-runs them against the latest `develop` before anything lands. The reviewer cites the CI run and doesn't re-run it. **Local reproduction is required only for what CI doesn't cover:** - storage layout and upgrade safety in `foundry/src/**` - deploy and upgrade scripts in `foundry/script/**` @@ -95,7 +96,7 @@ Templates: [`.claude/skills/din-pr-review/templates/`](../.claude/skills/din-pr- ### 4.4 Merge paths -- **(A) GitHub-side merge commit — the default.** The reviewer approves, CI is green, the branch is up to date, and the reviewer merges with a merge commit. If the reviewer wants something changed first, they push the fix onto the PR branch (or open a small follow-up PR). The last-push rule then sends the final approval to someone else. +- **(A) GitHub-side merge commit — the default.** The reviewer approves, CI is green, and the reviewer clicks **Merge when ready** (or `gh pr merge `) to add the PR to the merge queue. The queue re-runs `CI OK` on the PR merged onto the latest `develop` and lands it with a merge commit. If that run fails, the queue drops the PR and leaves `develop` untouched; fix it on the PR branch and queue it again. If the reviewer wants something changed first, they push the fix onto the PR branch (or open a small follow-up PR). The last-push rule then sends the final approval to someone else. - **(B) Local merge plus follow-up fix commit — exceptional, Umer only.** For PRs that need adapting against a `develop` that has moved (renames, ABI regeneration, docs drift). It needs the admin bypass. **The admin bypass** is for emergencies and path B only. Each use gets a one-line reason in [Discussion 170](https://github.com/InfiniteZeroFoundation/DevNet/discussions/170), followed by a normal reviewed PR where possible. @@ -191,6 +192,6 @@ The `din-*` Claude Code skills under [`.claude/skills/`](../.claude/skills/) imp | Close an issue as completed, or post done vs. remaining ([No. 5.2](#52-issues-status-updates-and-closing)) | `din-close-issue` | | Close a task Discussion ([No. 5.3](#53-tasks)) | `din-task-close` | -**When a local build is needed before merging.** A PR that merges as-is needs no local build: its required `CI OK` runs the full pipeline on GitHub's merge of the PR into `develop`, and the branch has to be up to date, so that run tests exactly what lands. When a reviewer applies fixes, they run `din-merge-build-test` on the fixed branch before pushing it, so the PR doesn't go red in public. CI then re-runs on the pushed fixes and is the gate again. In path B there's no PR CI for the deviation commit, so the local run and `din-push-develop`'s precheck are the gate. +**When a local build is needed before merging.** A PR that merges as-is needs no local build: its required `CI OK` runs the full pipeline on GitHub's merge of the PR into `develop`, and the merge queue runs it again on the exact commit that lands. When a reviewer applies fixes, they run `din-merge-build-test` on the fixed branch before pushing it, so the PR doesn't go red in public. CI then re-runs on the pushed fixes and is the gate again. In path B there's no PR CI for the deviation commit, so the local run and `din-push-develop`'s precheck are the gate. **Use the skills.** Maintainers do every step in the table above through its skill: PR reviews, review fixes, merges, direct pushes, issue approvals, issue closures and task closures. Each skill runs the peer check and the verification steps this file requires, so the record looks the same whoever does the work. Working by hand is the fallback for when the skills can't run (no Claude Code, a tool outage). Then follow this file and the [templates](../.claude/skills/din-pr-review/templates/) exactly, including running `din_peer_check.py` first. From 079cacb9f86e309b3c261febbb80a2416a9da6b6 Mon Sep 17 00:00:00 2001 From: umermjd11 Date: Mon, 2 Nov 2026 20:48:10 +0500 Subject: [PATCH 2/2] docs(skills): address PR No. 310 review; enqueue via GraphQL - 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 --- .claude/skills/din-pr-merge/SKILL.md | 21 ++++++++++++++------- .claude/skills/din-pr-review/SKILL.md | 9 ++++++--- Developer/MAINTAINING.md | 2 +- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/.claude/skills/din-pr-merge/SKILL.md b/.claude/skills/din-pr-merge/SKILL.md index c019a03..009507a 100644 --- a/.claude/skills/din-pr-merge/SKILL.md +++ b/.claude/skills/din-pr-merge/SKILL.md @@ -111,13 +111,17 @@ If any gate fails, report which one and stop. Don't use `--admin`. ### A3. Merge -On the operator's go-ahead, add the PR to the merge queue. `develop` -requires the queue, so `gh pr merge` queues it rather than merging directly, -and the queue picks the merge method (merge commit). Don't pass `--admin`, -which would skip the queue. +On the operator's go-ahead, add the PR to the merge queue (the queue merges +with a merge commit). Don't use `gh pr merge`: with auto-merge disabled on the +repo it fails with "Auto merge is not allowed for this repository", and +`--admin` would skip the queue. Enqueue through GraphQL instead (the same as +clicking **Merge when ready**): ```bash -gh pr merge -R $R +PR_ID=$(gh api graphql -f query="{repository(owner:\"InfiniteZeroFoundation\",name:\"DevNet\"){pullRequest(number:){id}}}" \ + --jq .data.repository.pullRequest.id) +gh api graphql -f query='mutation($p:ID!){enqueuePullRequest(input:{pullRequestId:$p}){mergeQueueEntry{state position}}}' \ + -f p="$PR_ID" ``` The queue builds a temporary `gh-readonly-queue/develop/...` branch (latest @@ -135,8 +139,11 @@ gh pr view -R $R --json state,mergedAt,mergeCommit --jq '{state, mergedAt, s re-queue once it's fixed. A fix pushed to the branch needs a fresh approval 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). + and tell Umer. The rollback (Discussion 208) is two changes to the + `develop` ruleset together: turn the merge-queue rule off **and** turn + "Require branches to be up to date before merging" back on, so merged PRs + are still tested against the latest `develop`. Then revert this skill and + `MAINTAINING.md` to the up-to-date wording. --- diff --git a/.claude/skills/din-pr-review/SKILL.md b/.claude/skills/din-pr-review/SKILL.md index c95b2c0..a55df54 100644 --- a/.claude/skills/din-pr-review/SKILL.md +++ b/.claude/skills/din-pr-review/SKILL.md @@ -115,9 +115,12 @@ locally only what CI doesn't cover:** locally (MAINTAINING No. 4.9) - any claimed behaviour that no test exercises -If CI on the PR is red, stale against `develop`, or never ran, CI isn't -evidence: say so, and either ask the author to update the branch or run the -relevant suite yourself. +If CI on the PR is red or never ran, CI isn't evidence: say so, and either ask +the author to fix it or run the relevant suite yourself. A PR whose branch is +merely behind `develop` is fine: don't ask for an update, since the merge queue +re-runs CI on the latest `develop` before it lands. Ask the author to merge +`develop` in only when there's a real conflict, or when `develop` gained +something the PR's change depends on and you need to review against it. When you do run things locally: diff --git a/Developer/MAINTAINING.md b/Developer/MAINTAINING.md index 336e9c1..cf0f031 100644 --- a/Developer/MAINTAINING.md +++ b/Developer/MAINTAINING.md @@ -96,7 +96,7 @@ Templates: [`.claude/skills/din-pr-review/templates/`](../.claude/skills/din-pr- ### 4.4 Merge paths -- **(A) GitHub-side merge commit — the default.** The reviewer approves, CI is green, and the reviewer clicks **Merge when ready** (or `gh pr merge `) to add the PR to the merge queue. The queue re-runs `CI OK` on the PR merged onto the latest `develop` and lands it with a merge commit. If that run fails, the queue drops the PR and leaves `develop` untouched; fix it on the PR branch and queue it again. If the reviewer wants something changed first, they push the fix onto the PR branch (or open a small follow-up PR). The last-push rule then sends the final approval to someone else. +- **(A) GitHub-side merge commit — the default.** The reviewer approves, 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 PR merged onto the latest `develop` and lands it with a merge commit. If that run fails, the queue drops the PR and leaves `develop` untouched; fix it on the PR branch and queue it again. If the reviewer wants something changed first, they push the fix onto the PR branch (or open a small follow-up PR). The last-push rule then sends the final approval to someone else. - **(B) Local merge plus follow-up fix commit — exceptional, Umer only.** For PRs that need adapting against a `develop` that has moved (renames, ABI regeneration, docs drift). It needs the admin bypass. **The admin bypass** is for emergencies and path B only. Each use gets a one-line reason in [Discussion 170](https://github.com/InfiniteZeroFoundation/DevNet/discussions/170), followed by a normal reviewed PR where possible.