From b06a029be48babbc952529e516e5866742f225dc Mon Sep 17 00:00:00 2001 From: umermjd11 Date: Fri, 6 Nov 2026 22:49:53 +0500 Subject: [PATCH 1/2] docs(maintaining): codify S5 retro items 1-3 from Discussion 208 - No. 4.4: whoever gives the final approval queues the PR, or hands that step to a named eligible maintainer; Umer may queue PRs whose second sign-off came last, otherwise the peer reviewer does. - No. 4.1: the 2-working-day window covers second sign-offs and re-reviews, as a time to respond or escalate in; second keys waiting too long are routed to a security reviewer (default @Abidoyesimze). - No. 4.1: fewest-reviews tie-breaker for first reviews, among eligible reviewers only; skillset, availability and recusal still apply. Co-Authored-By: Claude Opus 5.5 --- Developer/MAINTAINING.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Developer/MAINTAINING.md b/Developer/MAINTAINING.md index cf0f031..9ac56a0 100644 --- a/Developer/MAINTAINING.md +++ b/Developer/MAINTAINING.md @@ -58,7 +58,14 @@ Tag reviewers by skillset: ### 4.1 Picking the reviewer -CODEOWNERS requests reviewers when you open the PR. Check that at least one requested reviewer isn't you or a co-author, and add one by hand if not. The reviewer either takes the PR or replies within **2 working days** with "can't take this — @umeradl". Silence past 2 working days escalates to Umer. There's no fixed rotation. We'll revisit that if third-party PRs stall. +CODEOWNERS requests reviewers when you open the PR. Check that at least one requested reviewer isn't you or a co-author, and add one by hand if not. The reviewer either takes the PR or replies within **2 working days** with "can't take this — @umeradl". Silence past 2 working days escalates to Umer. + +**Tie-breaker for first reviews.** There's no fixed rotation. When more than one maintainer in the [reviewer pool](#3-reviewer-pool) is eligible and has the skillset for a PR, the first review goes to whoever has done the fewest reviews that week. Review count doesn't capture effort, so also weigh each person's outstanding reviews and how complex they are, and their availability. The tie-breaker only chooses among eligible reviewers: the skillset table in [No. 3](#3-reviewer-pool) still decides who is eligible, and recusal or escalation is still the right answer when a PR is outside your depth. + +**The 2-working-day window covers every review request**, not just the first response: the `foundry/src/**` second sign-off ([No. 4.2](#42-review-depth)) and re-reviews after the author pushes new commits (which dismiss stale approvals, [No. 2](#2-what-github-enforces)) count too. It's a window to respond or escalate in, not a deadline to finish the review: + +- If a review needs more time, the reviewer says so on the PR within the window and states what's left to check. +- If a second sign-off has waited more than 2 working days, the peer reviewer pings it on the PR. Umer or Abraham can then route it to a security reviewer ([No. 4.2](#42-review-depth)), by default @Abidoyesimze. ### 4.2 Review depth @@ -96,7 +103,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** 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 approver queues by default:** whoever gives the final required approval queues the PR once every required check and sign-off is in, unless their review says otherwise. If they can't queue it, or aren't eligible to merge it under [No. 1](#1-the-rule) (for example, they co-authored it), they hand the step to a named eligible maintainer in the review, so no approved PR sits without an owner. When the last thing in is a `foundry/src/**` second sign-off from Umer or Abraham, Umer may queue it himself, and otherwise the peer reviewer does. 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. From 35ec74a70f3e08b66a04a6b771d4649ecb0b5075 Mon Sep 17 00:00:00 2001 From: umermjd11 Date: Sun, 8 Nov 2026 21:29:38 +0500 Subject: [PATCH 2/2] docs(maintaining): keep foundry/src second sign-off independent of the peer For @robertocarlous and @Abidoyesimze PRs touching foundry/src/**, the second sign-off is always @umeradl or @abrahamnash. Routing to a security reviewer applies only to other authors, and that reviewer can't be the PR's author or its peer reviewer. Drops the "by default @Abidoyesimze" fallback, which would have made him both peer and second sign-off on Roberto's PRs, or the second sign-off on his own (Codex review No. 1). Co-Authored-By: Claude Opus 5.5 --- Developer/MAINTAINING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Developer/MAINTAINING.md b/Developer/MAINTAINING.md index 9ac56a0..23532ae 100644 --- a/Developer/MAINTAINING.md +++ b/Developer/MAINTAINING.md @@ -65,11 +65,11 @@ CODEOWNERS requests reviewers when you open the PR. Check that at least one requ **The 2-working-day window covers every review request**, not just the first response: the `foundry/src/**` second sign-off ([No. 4.2](#42-review-depth)) and re-reviews after the author pushes new commits (which dismiss stale approvals, [No. 2](#2-what-github-enforces)) count too. It's a window to respond or escalate in, not a deadline to finish the review: - If a review needs more time, the reviewer says so on the PR within the window and states what's left to check. -- If a second sign-off has waited more than 2 working days, the peer reviewer pings it on the PR. Umer or Abraham can then route it to a security reviewer ([No. 4.2](#42-review-depth)), by default @Abidoyesimze. +- If a second sign-off has waited more than 2 working days, the peer reviewer pings @umeradl and @abrahamnash on the PR. For PRs from other authors, Umer or Abraham can instead route it to a security reviewer ([No. 4.2](#42-review-depth)) who isn't the PR's author or its peer reviewer. ### 4.2 Review depth -- **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. +- **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. When the author is @robertocarlous or @Abidoyesimze, the second sign-off is always @umeradl or @abrahamnash. For other authors, Umer or Abraham may route it to a security reviewer who isn't the PR's author or its peer reviewer. 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 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:**