Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .claude/skills/din-merge-build-test/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. |

Expand Down
43 changes: 34 additions & 9 deletions .claude/skills/din-pr-merge/SKILL.md
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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.

Expand Down Expand Up @@ -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 <N>`. 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.
Expand All @@ -111,14 +111,39 @@ 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 (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 <N> -R $R --merge
PR_ID=$(gh api graphql -f query="{repository(owner:\"InfiniteZeroFoundation\",name:\"DevNet\"){pullRequest(number:<N>){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
`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 <N> -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 (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.

---

Expand Down
13 changes: 8 additions & 5 deletions .claude/skills/din-pr-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
Santiagocetran marked this conversation as resolved.
Resolved
(`gh pr checks <N>`, link the job) instead of re-running it. **Reproduce
locally only what CI doesn't cover:**

Expand All @@ -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:

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion .claude/skills/din-push-develop/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
9 changes: 5 additions & 4 deletions Developer/MAINTAINING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)).
Expand Down Expand Up @@ -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/**`
Expand Down Expand Up @@ -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** 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.
Expand Down Expand Up @@ -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.
Loading