Shared maintainer workflow: peer review, merging, and issue/task closure (@Santiagocetran, @robertocarlous)

[umeradl]

@Santiagocetran @robertocarlous — cc @abrahamnash

Until now I (umeradl) have done all the administrative work on this repo myself: reviewing PRs, merging into develop, approving and closing issues, writing and closing task files and their Discussions, posting status updates, and keeping the wiki up to date. That doesn't scale, and it makes me a single point of failure. I want both of you to share this work with me. Your access will be raised to match.

This thread proposes how that works. Please read it, then reply against the numbered items (write "No. 3", not #3, so GitHub doesn't turn it into an issue link). Nothing changes on the repo until we've agreed on the details here.

---

1. The one rule everything else follows from

Nobody approves, merges or closes their own work. A peer does it.

Specifically:

Activity	Author / assignee may do it?	Who does it
Review a PR (verification + merge-proposal comments)	❌	A peer who didn't write it
Approve a PR (GitHub "Approve")	❌ (GitHub already blocks this)	A peer
Merge a PR into develop	❌	The peer who reviewed it, or me
Approve an issue's plan (Approved label)	❌ if you opened it or will implement it	A peer
Post a status update on an issue/task	✅ progress notes are fine	Anyone; the authoritative done-vs-remaining check is the peer's
Close an issue as completed	❌ if you implemented it	A peer
Close a task Discussion as Resolved	❌ if it's assigned to you	A peer
Write a new task file	✅	Anyone, but assigning it and closing it are done by someone else
Wiki edits	✅ for small fixes	A peer reviews anything substantive (see No. 5.5)

With two of you, "a peer" in practice means: Robbert's work → Santiago; Santiago's work → Robbert; if the peer is unavailable or it's outside their depth → me.

---

2. Where we are today

What the repo looks like right now:

• Access: you both have Triage. That covers labels, assigning, and closing and reopening issues and PRs. It does not let you merge PRs or push branches to this repo, and a restricted wiki needs push access to edit. A review from a Triage user also doesn't count toward a required approval.
• develop protection: the only requirement is the CI OK status check. No reviews are required, and admins aren't subject to the rules. That's why my local-merge-and-push flow works today.
• main: not protected at all.
• No CODEOWNERS file.
• Process lives in my local Claude skills (committed under .claude/skills/): din-pr-review, merge-fix-comment-update, din-merge-build-test, din-pr-merge, din-push-develop, din-approve-issue, din-close-issue, din-task-close. Several are hard-coded to "must run as umeradl" and to my commit trailers, so you can't use them as they are.

Most of this rule (No. 1) can't be enforced by GitHub today. The proposals below fix that wherever GitHub can enforce it, and use convention everywhere else.

---

3. Proposed access and enforcement

No. 3.1 — Raise both of you to Write. That lets you merge PRs, push feature branches to this repo (no more fork-only), close Discussions, and edit the wiki. Maintain/Admin stay with me and Abraham. Repo settings, rulesets and secrets don't need to be shared.

No. 3.2 — Add a ruleset on develop so the peer rule is enforced by GitHub, not just by trust:

• Require a pull request before merging.
• 1 required approval.
• Require approval of the most recent reviewable push. This one matters: if the reviewer pushes fix commits onto your PR, they can no longer approve it themselves, and someone else has to.
• Dismiss stale approvals when new commits are pushed.
• Keep CI OK required. Block force-pushes and deletion.
• Bypass list: only me (for emergency CI fixes and the local-merge flow, see No. 4.4). See question No. 9.1 on whether I should keep that bypass at all.

No. 3.3 — Add .github/CODEOWNERS so the right reviewer is requested automatically:

• foundry/src/**, foundry/script/** → @robertocarlous + @umeradl
• dincli/**, tests/**, docker/ops/IPFS → @Santiagocetran + @umeradl
• Documentation/**, Developer/** → both of you

CODEOWNERS only requests reviewers. I'm not proposing "require code-owner review", because it would deadlock: Robbert couldn't get a contract PR merged while he's the only contract owner. The real gate is the 1-approval rule plus the peer rule.

No. 3.4 — Protect main too (PR required, CI OK, no force-push). Releasing to main stays with me for now (No. 6).

---

4. PR review and merge workflow

No. 4.1 — Picking the reviewer. When you open a PR, request your peer as reviewer (CODEOWNERS will do this automatically once added). The peer either takes it or replies within 2 working days with "can't take this — @umeradl". Silence past 2 working days escalates to me automatically.

No. 4.2 — Depth of review: contracts get two keys. Any PR touching foundry/src/** (contract logic, storage layout, upgrade paths) needs the peer's review plus a second sign-off from me or a security reviewer (e.g. @Abidoyesimze for audit-style checks). Santiago reviewing a Solidity change on his own isn't the bar we want before a fresh DevNet deploy. Docs-only, test-only, Python and tooling PRs need one peer.

No. 4.3 — Same 3-comment record as today, written by the reviewer:

1. Verification comment. Reproduce every checkable claim by running it, not by reading it ("Claimed / Verified" pairs). Never accept "done/landed" in a comment as proof.
2. Merge-proposal table. Per file: Change, Lines, Diff, Functionality (how & why), diff vs current develop, recommended merge proposal, actual merge proposal, pending proposal, local/GitHub conflict. "Actual merge proposal" is filled in by whoever performs the merge, not pre-filled at review time.
3. Outcome comment. Posted after the merge lands, with the real merge SHA(s), the files that deviated from the PR, and final build/test numbers.

The templates live in .claude/skills/din-pr-review/templates/. Use them with or without Claude.

No. 4.4 — Two merge paths, and when to use each:

• (A) GitHub-side merge (default for you). Reviewer approves, CI is green, and the reviewer merges with a merge commit (not squash, so authorship and history survive). The develop ruleset enforces everything.
• (B) Local merge + follow-up fix commit (what I do today). Use it when the PR needs adaptation against a develop that has moved (renames, ABI regeneration, docs drift). Under the ruleset this needs bypass, so for now only I do it. Proposal: if a reviewer wants to change something before merging, they push the fix onto the PR branch (or open a small follow-up PR). The "most recent push" rule then sends the final approval back to the other person. This keeps the record on GitHub instead of in a local merge.

No. 4.5 — Attribution. Merges keep the PR author as the commit author. Any deviation commit the merger writes credits the PR author with Co-Authored-By:. Never squash away someone's authorship.

No. 4.6 — After the merge: CI on develop. Whoever merged owns that push's CI run, so this is no longer just me. If it's red, the merger fixes it on top or reverts within the same working day. The existing incident thread, Discussion 170, stays the log, and its "~6h + commits on top" escalation rule applies to everyone, including me.

No. 4.7 — Reverts. Any maintainer may revert a merge that breaks develop without waiting for the author. The revert goes through a PR and peer approval like anything else, unless it's me using the emergency bypass. The author re-lands the change in a new PR.

No. 4.8 — Labels. Keep using the existing ones consistently: Reviewed once comments 1–2 are posted, Merged Locally / merged / Partially Merged after the merge, Hurry to Merge / No Hurry To Merge for priority, Draft, Deferred.

---

5. Other administrative workflows

No. 5.1 — Issues: approval before implementation. A new non-trivial issue gets a verdict comment (Approved / Approved with amendments / Changes requested / Not approved) before anyone writes code. The verdict checks every factual claim (file:line refs, "function X doesn't exist", "the ABI already has Y") against origin/develop. On approval, add the Approved label. The approver can't be the issue's author or its intended implementer. Mechanism-design issues (slashing, tokenomics, fees, scoring) still need my or Abraham's sign-off, because they're product decisions, not code-review ones.

No. 5.2 — Issues: status updates and closing. Anyone may post progress. Closing as completed needs a peer check of every proposed change, every approval amendment, every verification item and every linked PR against origin/develop, followed by a closing comment listing what landed and where (SHAs). If anything remains, post a "done vs. remaining" status comment and leave the issue open. Closing as not planned / duplicate: anyone may do it, with a one-line reason and a link to the duplicate.

No. 5.3 — Tasks (Developer/tasks/task_ddmmyy_N.md + a Tasks-category Discussion).

• Writing a task: anyone may draft one, following the existing header format (ID / Assigned to / Created / Status / Roadmap ref / Dates). Task files land on develop through a PR like any other file, so they get peer review too.
• Assigning: the assignee doesn't assign themselves without me or the peer agreeing in the Discussion. This keeps scope and timelines honest. Anything involving compensation or a new contributor stays with me and Abraham.
• Status updates: the assignee posts progress in the task Discussion. The peer posts the authoritative "verified done vs. remaining" check.
• Closing: a peer verifies every deliverable in the task file's checklist and every PR it names against develop, then closes the Discussion as Resolved with a closing comment and updates the task file's Status: line in a follow-up PR. A task can't close while a PR it depends on is still open or in draft.

No. 5.4 — Docs syncing on merge. Whoever merges a PR checks that Documentation/ still describes develop. They also check that bundled ABIs (dincli/abis/) were regenerated if a contract interface changed, and that Developer/BACK_LOG.md / ROADMAP.md entries the PR resolves are updated, or that a follow-up issue is filed.

No. 5.5 — Wiki. The wiki has no PR mechanism, so:

• Typos, broken links, and syncing a page to something already merged on develop: just edit it.
• New pages, restructuring _Sidebar.md, or anything describing a mechanism: post the draft (or a diff) as a comment in a Discussion and get a peer 👍 before publishing.
• Conventions: links into the repo use blob/develop/...; planned components carry the 📋 Planned banner; no assignee names or week-level dates on public pages; pages end with "Further reading" links into Documentation/.

No. 5.6 — Discussions housekeeping. Anyone may answer, label, or lock stale threads. Closing a design/Ideas Discussion as decided needs a comment that records the decision and where it was implemented. Mechanism decisions are recorded by me or Abraham.

---

6. What stays with me (for now)

• Pushing to or releasing on main.
• Anything on Sepolia or other live networks: platform deploys, upgrades, DIN-Representative keys and actions, slasher authorisations.
• Repo settings, rulesets, secrets, access changes.
• Contributor onboarding, compensation, and HR-side task approval (with Abraham).
• Roadmap priority calls and final mechanism-design decisions (with Abraham).
• The emergency bypass on develop (see No. 9.1).

---

7. Edge cases

• Co-authored PRs (both of you committed to it): neither of you may approve it. It goes to me.
• Reviewer pushes substantial fixes onto a PR: they've become a co-author. Under the "most recent push" rule, the original author or I give the final approval, and the reviewer doesn't merge their own fixes unreviewed.
• Trivial changes (typo, dead link, comment fix): still a PR, still 1 approval. A quick 👀 approval is fine; skip the full 3-comment record and note "trivial" in the approval.
• Disagreement between reviewer and author: discuss it on the PR. If you're not converging after a round or two, tag me and I decide. Nobody merges over an unresolved "Changes requested".
• Urgent fix while the peer is offline (e.g. a red develop blocking everyone): open the PR, tag both the peer and me. Whoever answers first approves it. Never merge unreviewed.
• PRs from other contributors (Similoluwa, external forks): either of you may review and merge, provided you didn't co-author them. The foundry/src two-key rule (No. 4.2) still applies.
• My own PRs (from the umermjd11 fork): see No. 9.2.

---

8. Tooling changes I'll make (once we agree)

1. Raise both of you to Write (No. 3.1).
2. Add the develop and main rulesets (No. 3.2 / No. 3.4) and .github/CODEOWNERS (No. 3.3).
3. Generalise the din-* Claude skills: drop the hard-coded "must run as umeradl", attribute trailers to whoever runs them, and add a self-check that refuses to approve, merge or close when the active gh user is the author or assignee. I'll also commit din-close-issue, which isn't in the repo yet.
4. A short Developer/MAINTAINING.md that summarises whatever we agree here, linked from CONTRIBUTING.md, so the process lives in the repo and not only in this thread.

Rollout: for the first ~2 weeks I'll shadow. You review and merge, and I read every merged PR after the fact and comment if something was missed. After that, I step back to the role in No. 6.

---

9. Open questions — please reply

• No. 9.1 — Should I keep an admin bypass on develop (emergency CI fixes, local merge path B), or should I go through the same 1-approval rule as everyone else?
• No. 9.2 — My own PRs from umermjd11: should they now also require one of you to approve, instead of me reviewing them as umeradl? I lean yes.
• No. 9.3 — Is a 2-working-day review response window realistic for both of you? Do you want a rotation, e.g. one of you is "on review" each week for third-party PRs?
• No. 9.4 — Should Similoluwa be added as a (non-merging) required second reviewer for foundry/src changes, or should that second key stay with me?
• No. 9.5 — Merge path: are you comfortable with GitHub-side merge commits as your default (No. 4.4 A), or do you want to learn the local stash-merge-pop + follow-up-commit flow too?
• No. 9.6 — Anything in No. 6 you think should move to you sooner, or anything in No. 1 you think is too strict?

[Abidoyesimze]

I think this actually make sense and it's going to take some loads off you (Umer).
And in terms of the reviewers, what we just need to do is think about the cons and pros. The reviewers we are tagging to review each of our work skillset base on what we work on we'd need to be tagging reviewers base on their skillset.

[umeradl]

@robertocarlous @Abidoyesimze your skills are complementary, so you can review each other's PRs.

The objective of a PR review is to check that the PR does what it is intended to do, that it passes tests, and whether it needs any fixes, with one review pass before merge.

I do my reviews with Claude Code. A review pass with an AI agent in fact does this really well with skills such as din-pr-review. The objective is different: while doing development work, AI can miss certain things. It can introduce bugs or miss a critical detail, while in review we do the opposite.

[robertocarlous]

@umeradl Thanks for laying this out. I support the proposed direction and the peer-review principle. My thoughts:

No. 9.1: Keep the admin bypass for emergencies, but use it only when necessary and document the reason.
No. 9.2: Yes. Your PRs should receive approval from one of us, like everyone else’s.
No. 9.3: A 2-working-day response window seems reasonable. I don’t think a fixed rotation is necessary initially; we can revisit it if third-party PRs aren’t getting timely reviews.
No. 9.4: Keep the second sign-off with you for now. We can revisit adding another security reviewer if the review load or availability calls for it.
No. 9.5: I’m comfortable using GitHub-side merge commits as the default. The local merge path can remain limited to the cases described.
No. 9.6: I don’t have changes to suggest to No. 1 or No. 6 at this stage. The separation of responsibilities and the rule against approving or closing our own work seem appropriate.

I support moving ahead with the rollout and documenting the agreed process in Developer/MAINTAINING.md.

[abrahamnash]

Thanks @umeradl — this is a good proposal and I support the direction. @robertocarlous @Abidoyesimze, my answers:

No. 9.1: Keep the admin bypass with Umer for emergencies only. Use it rarely, document why, and follow up with a normal reviewed PR where possible.

No. 9.2: Yes — Umer’s PRs should require approval from one of you. No self-review.

No. 9.3: 2 working days is reasonable. No fixed rotation for now; revisit if third-party PRs stall.

No. 9.4: The second sign-off should stay with me or Umer. If neither of you has the depth for a particular foundry/src change, route it to us or bring in a security reviewer like @Abidoyesimze.

No. 9.5: GitHub-side merge commits as default is fine. Local merge stays exceptional.

No. 9.6: No changes to No. 1 or No. 6 from me.

The only thing I’d add is what @Abidoyesimze raised: reviewer tagging should be skillset-based. But just as important, if someone is tagged and feels it’s outside their skillset, they should flag it and ask for a second review rather than approve. That should be an explicit, valid outcome in MAINTAINING.md — better a recusal/escalation than a rubber-stamp.

If everyone agrees, go ahead with the tooling changes and the MAINTAINING.md draft.

[umeradl]

In the Friday meeting (Oct 2), we will do a round-table where each team member discusses their agentic AI-assisted dev work. Moving forward, we should make sure that everyone has a working (subscription) of CLI terminal version of either Claude Code, Codex, Muse, or Grok.

[Santiagocetran]

Hey Umer, I agree with distributing reviews and merges across the team. Our own PRs should be reviewed by someone else, and having every review and merge depend on you creates a bottleneck. I’d keep your bypass for genuinely necessary emergencies, with the normal path going through a PR and peer approval.

My main concern is the amount of manual verification expected from reviewers. Checking out a branch and testing locally can be useful, especially for risky changes or gaps in coverage, but I don’t think reproducing every check locally should be mandatory for every PR. Otherwise, we could spread the bottleneck rather than really reduce it.

I think this workflow change should go hand in hand with improving CI. We already have integration tests using a local chain, IPFS and Docker, but the current Python CI job excludes them. Bringing those into a reproducible CI environment would give us stronger evidence that the components actually work together, alongside the existing unit and contract tests.

From my side, stronger CI could help make the review process you’re proposing more manageable. Authors would provide a clear explanation and relevant tests, while reviewers focus on the implementation, design, edge cases and coverage, leaving a concise explanation of their findings and any requested fixes. Required checks should pass against an up-to-date develop base before merging, with local testing where additional verification is needed. Passing CI would support the reviewer’s judgment, rather than replace it.

For what happens after that, I think it would be useful to distinguish:

• Post-merge checks, where needed, to validate the actual code that landed on develop, without necessarily repeating the entire expensive test suite
• A shared develop/testnet environment where we can exercise real workflows, debug and test how our changes work together during development
• A separate validation stage for PRs from develop to main, checking the proposed release before it’s promoted. That could use a more controlled environment representing the final workflow

Would that fit with the direction you have in mind? I think it would help us automate repeatable verification while keeping room for hands-on testing and a clear check before release.

For failures after a merge, I agree the merger should make sure they’re addressed promptly, but I’d keep the author involved in fixing their change, with a clear fallback if either person is unavailable.

I can commit to working on the CI improvements tomorrow and over the weekend, with at least an initial PR open by Monday. We could introduce shared reviews alongside those improvements and keep targeted manual checks for the gaps until they’re covered.

And yes, happy to discuss our AI/agentic development environments tomorrow and share my workflow!